chore: Updating List item and List components in ADS to start using it in Entity explorer #38344
Conversation
WalkthroughThis pull request introduces significant modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
ankitakinger
changed the title
github-actions
bot
added
IDE Pod
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12515753321. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/List/List.styles.tsx (1)
39-91: LGTM with minor type safety enhancementThe StyledListItem implementation looks good with proper styling variables and accessibility considerations.
Consider using a literal type for rightControlVisibility instead of string:
- rightControlVisibility?: "hover" | "always"; + rightControlVisibility?: RightControlVisibility; +type RightControlVisibility = "hover" | "always";app/client/packages/design-system/ads/src/List/List.tsx (1)
Line range hint
87-92: Props update looks goodThe transition from endIcon to rightControl provides more flexibility.
Consider adding JSDoc comments to document the new props:
/** Content to be rendered in the right control area */ rightControl?: React.ReactNode; /** Controls the visibility behavior of the right control. Default: "always" */ rightControlVisibility?: "hover" | "always";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/packages/design-system/ads/src/List/List.styles.tsx(4 hunks)app/client/packages/design-system/ads/src/List/List.tsx(4 hunks)
🔇 Additional comments (4)
app/client/packages/design-system/ads/src/List/List.styles.tsx (2)
Line range hint 94-113: Clean implementation
The wrapper components are well-structured with appropriate flex layout properties.
114-117: LGTM
Simple and focused implementation for the right control container.
app/client/packages/design-system/ads/src/List/List.tsx (2)
113-121: Well-structured with proper accessibility
Good implementation with appropriate data attributes and keyboard navigation support.
162-164: Clean implementation
Straightforward conditional rendering of the right control content.
ankitakinger
added
ok-to-test
|
Deploy-Preview-URL: https://ce-38344.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/List/List.styles.tsx (1)
Line range hint
54-89: LGTM with a minor suggestion for line height consistencyThe layout improvements look good. However, there's an inconsistency in line height values - some use pixels (16px) while others use
normal.Consider standardizing line heights:
- line-height: 16px; + line-height: normal;app/client/packages/design-system/ads/src/List/List.tsx (1)
Line range hint
122-165: Consider extracting description rendering logicThe content rendering logic is getting complex. Consider extracting the description rendering into separate components for better maintainability.
Example structure:
const BlockDescription = ({ description }: { description: string }) => ( <TextWithTooltip className={ListItemBDescClassName} color="var(--ads-v2-color-fg-muted)" isMultiline kind="body-s" > {description} </TextWithTooltip> ); const InlineDescription = ({ description }: { description: string }) => ( <TextWithTooltip className={ListItemIDescClassName} color="var(--ads-v2-color-fg-muted)" kind="body-s" > {description} </TextWithTooltip> );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/packages/design-system/ads/src/List/List.styles.tsx(4 hunks)app/client/packages/design-system/ads/src/List/List.tsx(4 hunks)
🔇 Additional comments (4)
app/client/packages/design-system/ads/src/List/List.styles.tsx (2)
91-99: LGTM! Well-structured list container
The flex layout with proper spacing and overflow handling is well implemented.
Line range hint 101-159: LGTM! Excellent implementation of data attributes
The component follows best practices by:
- Using data attributes for conditional styling
- Implementing proper focus management
- Handling hover/active states cleanly
app/client/packages/design-system/ads/src/List/List.tsx (2)
87-88: LGTM! Good replacement for endIcon
The rightControl prop provides more flexibility than the previous endIcon implementation.
113-121: LGTM! Well-structured list item implementation
Good implementation of data attributes and accessibility features.
…o chore/entity-list-item
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx (2)
18-18: Keep an eye on accessibility for invisible elements.
Opacity set to0might make descriptions inaccessible to screen readers. Consider visually hiding these elements in a more accessible manner if required.
22-22: Ensure a suitable fallback for non-hover devices.
Relying on:hovermay degrade user experience on touchscreens. Consider a different approach for mobile use cases.app/client/packages/design-system/ads/src/List/List.tsx (1)
121-121: Rethink clickable non-button elements.
If this wrapper acts like a button, consider<button>or appropriate ARIA roles for better accessibility.
📜 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.styles.tsx(4 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)app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/Group.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ce/pages/Editor/IDE/EditorPane/Query/hooks.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/ads/src/List/List.styles.tsx
- app/client/packages/design-system/ads/src/List/List.types.tsx
🔇 Additional comments (8)
app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (1)
51-51: Check for legacy references to wrapperClassName.
It looks good that you've updated wrapperClassName to the standard React className property. However, ensure that no existing code references the old property.
You can run the following script to identify any residual references:
app/client/packages/design-system/ads/src/List/List.tsx (7)
9-9: Good addition for the new rightControl approach.
This import neatly supports the shift from endIcon to a flexible rightControl.
86-87: rightControl and rightControlVisibility are a nice enhancement.
Replacing the old endIcon with these props increases versatility. Great work.
112-120: Consider aligning data attributes with accessibility requirements.
Using data- attributes for styling is fine, but confirm if you need ARIA attributes or roles for better screen reader compatibility.
125-126: Retaining startIcon is fine.
No issues spotted: it aligns with existing patterns without conflict.
158-159: No concerns on bracket closure changes.
Everything appears consistent here.
160-161: Optional right control is handled cleanly.
Conditionally rendering rightControl is a good approach for flexibility.
163-163: Closing tag update is straightforward.
No issues found.
Description
Under Entity explorer modularisation, we are updating the List item and List components in ADS which will eventually be getting used in Entity explorer to show the entity items. This change makes sure current usages of this component doesn't break.
Fixes #37620
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12517459729
Commit: e1601a6
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Sanity/Datasources/MockDBs_Spec.ts
List of identified flaky tests.Fri, 27 Dec 2024 17:09:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
rightControlproperty for list items, enhancing interactivity.rightControlVisibilityproperty to manage control visibility based on hover state.customTitleComponentproperty for additional customization options.Bug Fixes
Style
Documentation