-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: [ADS] Entity Tree Hover interactions #38615
Conversation
WalkthroughThe pull request introduces styling and configuration modifications across several design system components, primarily focusing on list items, entity explorers, and input styling. The changes involve removing focus styles, adjusting input element properties, and refining selection and disabled state behaviors in list components. These updates aim to streamline the visual presentation and interaction model of UI elements. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12743941314. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.styles.ts (2)
43-54
: Consider improving comment clarityThe comment structure could be more explicit about the styling hierarchy between wrapper and list item.
- /* selected style */ - // Set the selected style for wrapper and remove from list item + /* Selected state styling: + * - Background color is set on the wrapper + * - List item's background is reset to prevent conflicts + */
57-69
: Maintain comment consistency with selected stateSimilar to the selected state, the comment structure could be improved.
- /* disabled style */ - // Set the disabled style for wrapper and remove from list item + /* Disabled state styling: + * - Opacity and background color are set on the wrapper + * - List item's background is reset to prevent conflicts + */app/client/packages/design-system/ads/src/List/List.stories.tsx (1)
41-41
: Consider adding more selection test casesWhile the current example is good, consider adding multiple selected items or disabled+selected combination to demonstrate all possible states.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/packages/design-system/ads/src/List/List.stories.tsx
(1 hunks)app/client/packages/design-system/ads/src/List/List.styles.tsx
(0 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts
(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
(0 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.styles.ts
(3 hunks)
💤 Files with no reviewable changes (2)
- app/client/packages/design-system/ads/src/List/List.styles.tsx
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (3)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts (1)
17-23
: Good move shifting styles to CSS!Moving styles from component props to CSS is a cleaner approach. The use of design system variables (
--ads-v2-color-bg
) ensures consistency, and the explicit dimensions maintain proper alignment with the parent element.app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityListTree/EntityListTree.styles.ts (2)
3-3
: LGTM! Import is correctly used for style overrides.
37-37
: LGTM! Preventing text selection improves navigation UX.
Deploy-Preview-URL: https://ce-38615.dp.appsmith.com |
Description
Overrides List item styles in Entity Tree to avoid styling conflicts
Automation
/ok-to-test tags="@tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12743934091
Commit: c47b5fe
Cypress dashboard.
Tags:
@tag.IDE
Spec:
Mon, 13 Jan 2025 09:45:32 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit