chore: updating list item and list components in ADS #38344
Conversation
WalkthroughThis pull request introduces significant modifications to the List component across multiple files in the design system. The changes primarily focus on replacing the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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 skip-tests=true |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12479029057. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
app/client/packages/design-system/ads/src/List/List.types.tsx (2)
9-10: Consider renaming for clarity.“rightControl” is concise, but “endControl” or “actions” might better convey its purpose if it typically contains actions like icons or buttons.
11-12: Add JSDoc description.The
rightControlVisibilityproperty should include a more descriptive JSDoc comment that clarifies its role in controlling visibility.app/client/packages/design-system/ads/src/List/List.styles.tsx (4)
51-56: Use consistent control toggle.Hiding the
RightControlWrapperon hover is elegant, but confirm all pointer events still work seamlessly, especially for screen readers.
61-63: Active state styling.Check if the active state complements the final design. Sometimes an even darker shade clarifies active feedback.
65-67: Selected state.Reusing
active-bgfor selected state can be confusing. Consider a distinct color or style for persistent selection.
110-116: RightControlWrapper usage.This container is a valid approach for grouping interactive elements on the right side. Ensure clickable regions have adequate a11y attributes if needed.
app/client/src/pages/Editor/Explorer/Actions/ActionEntity.tsx (2)
24-24: Clean code preference.Commented-out import suggests the code was deprecated. Remove these lines if they're fully obsolete.
29-35: Remove stale code.These commented-out functions indicate a change in approach for updating action names. Delete them if they're not needed.
app/client/packages/design-system/ads/src/List/List.tsx (1)
129-136: Descriptive text usage.Displaying
titleviaTextWithTooltipis great. Confirm large titles are truncated gracefully.app/client/packages/design-system/ads/src/List/List.stories.tsx (1)
164-165: Good use of the new hover-based visibility.
Ensure that the hover interaction is clearly documented so users understand how the control rendering toggles.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/ads/src/List/List.stories.tsx(4 hunks)app/client/packages/design-system/ads/src/List/List.styles.tsx(3 hunks)app/client/packages/design-system/ads/src/List/List.tsx(3 hunks)app/client/packages/design-system/ads/src/List/List.types.tsx(2 hunks)app/client/src/pages/Editor/Explorer/Actions/ActionEntity.tsx(3 hunks)app/client/src/pages/Editor/Explorer/ExplorerIcons.tsx(1 hunks)
🔇 Additional comments (25)
app/client/src/pages/Editor/Explorer/ExplorerIcons.tsx (1)
269-270: Confirm that the new icon dimensions uphold the intended design.
The default height and width have been slightly reduced from 18px→16px and 36px→34px, respectively. Please ensure this aligns with design and layout requirements across the application.
app/client/packages/design-system/ads/src/List/List.types.tsx (2)
26-26: Good choice making description optional.
This makes the component more flexible for items without a description.
30-30: Consistent naming recommendation.
With descriptionType now optional, ensure conditioning logic consistently checks for undefined before usage.
app/client/packages/design-system/ads/src/List/List.styles.tsx (7)
39-41: Enhanced flexibility with typed prop.
Extending Wrapper to accept rightControlVisibility is a neat way to handle hover-based displays.
48-50: Ensure consistent sizing.
Adjusting the border-radius and min-height is fine, but confirm these changes match the rest of the design system’s guidelines for list items.
57-59: Hover color alignment.
This hover color is consistent with the design system’s hover states. Nice job!
69-74: Disabled overrides.
The reduced opacity and not-allowed cursor are standard patterns, which is good. Verify that hover or focus states don’t override them.
76-81: Focus outline clarity.
Ensuring a clear, accessible focus outline is essential. Great to see these focus-visible rules.
129-132: Spacing changes.
Adjusting padding and gap looks fine, but double-check it aligns with other design system components for seamless consistency.
159-159: Line-height update.
Increasing line-height improves readability; ensure it doesn’t break layout for multiline descriptions.
app/client/src/pages/Editor/Explorer/Actions/ActionEntity.tsx (3)
3-3: Import retention confirmation.
Confirm you still need EntityClassNames. If not, removing it will keep code tidy.
27-27: Align import sources.
Importing ListItem from "@appsmith/ads" is consistent with the new approach. Good job.
113-120: Variadic props usage.
Using the ListItem with rightControl and rightControlVisibility is consistent with the new design. Confirm the class name “action t--action-entity” remains relevant under new styling.
app/client/packages/design-system/ads/src/List/List.tsx (9)
8-8: Check import usage.
The newly added import for RightControlWrapper must match references in the file to avoid potential bundling issues.
87-88: Increased customizability.
Introducing rightControl and rightControlVisibility is a flexible alternative to a single endIcon. Good approach!
113-119: Proper data attributes.
Using data-disabled and data-selected is a clean way to handle styling states. Confirm that UI test frameworks reference these accurately.
127-128: Named handler.
Separating the click and keydown handlers fosters clarity. The inline arrow function is short and fine here.
138-140: Color usage for errors.
Using a direct CSS variable for error color is straightforward and consistent with the system.
146-147: Block vs inline logic.
Ensuring the block description is only rendered if it exists is correct. Great clarity in condition usage.
148-155: Inline description usage.
Similarly, this condition ensures inline descriptions are only displayed if present, which is clean and robust.
156-156: Fallback check.
rightControl presence is neatly handled with a short conditional. No complaints here.
158-159: Right control wrapper.
Logic is straightforward: if rightControl is defined, wrap it. This approach fosters modularity.
app/client/packages/design-system/ads/src/List/List.stories.tsx (3)
2-2: Great addition of the Button import.
It appears you’re utilizing the Button component for the right control. No concerns here.
82-86: Good introduction of the rightControl property.
The ReactNode type choice makes it suitably flexible for various controls.
217-224: Nice addition of the ListItemRightControlStory.
Your new story is consistent with the updated API, providing a clear example of using Button as the right control element.
app/client/packages/design-system/ads/src/List/List.stories.tsx
Outdated
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-38344.dp.appsmith.com |
…o chore/entity-list-item
…o chore/entity-list-item
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/List/List.stories.tsx (1)
82-96: Consider grouping related props togetherThe
customTitleComponentprop should be grouped with other visual/layout props liketitleandstartIconfor better organization of the API documentation.Also applies to: 149-156
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/List/List.stories.tsx(5 hunks)app/client/packages/design-system/ads/src/List/List.tsx(4 hunks)app/client/packages/design-system/ads/src/List/List.types.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/List/List.types.tsx
🔇 Additional comments (6)
app/client/packages/design-system/ads/src/List/List.tsx (4)
8-8: LGTM: Import addition for RightControlWrapper
87-88: LGTM: Props update for enhanced control flexibility
The replacement of endIcon with rightControl and rightControlVisibility provides better flexibility for list item controls.
113-119: LGTM: Enhanced wrapper attributes for better state management
Good addition of data attributes for disabled and selected states, along with rightControlVisibility prop.
162-164: LGTM: Clean implementation of rightControl rendering
app/client/packages/design-system/ads/src/List/List.stories.tsx (2)
2-2: LGTM: Import addition for Button component
173-173: LGTM: Comprehensive story examples for rightControl usage
Good examples showing both simple icon and button usage in rightControl.
Also applies to: 225-232
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 924ae26 yet
Wed, 25 Dec 2024 09:42:25 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
rightControlproperty for list items, enhancing interactivity.rightControlVisibilityproperty to control the display of right controls.Bug Fixes
Style
WrapperandStyledListItemcomponents, improving visual feedback and layout.Refactor
Entitycomponent with the newListItemcomponent in the action entity rendering.