chore: Replacing the entity group list component on state inspector and data side pane with new ADS component #38621
Conversation
…e pane with new ADS component
…o chore/replace-group-list-implementation
WalkthroughThe pull request introduces comprehensive changes to the design system and entity explorer components, focusing on enhancing list item rendering and group management. The modifications span multiple files, introducing a new Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (2)
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
|
github-actions
bot
added
IDE Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
app/client/packages/design-system/ads/src/List/List.tsx (1)
90-95: Consider simplifying boolean conversionsThe Boolean constructor is unnecessary here as the expressions already evaluate to boolean values.
- const isBlockDescription = Boolean( - descriptionType === "block" && description, - ); - const isInlineDescription = Boolean( - descriptionType === "inline" && description, - ); + const isBlockDescription = descriptionType === "block" && description; + const isInlineDescription = descriptionType === "inline" && description;app/client/packages/design-system/ads/src/List/List.styles.tsx (1)
94-94: Consider using CSS custom property for max-height value.The hardcoded max-height value could be moved to the Variables CSS block for consistency with other dimensional values in the design system.
- max-height: 32px; + max-height: var(--ads-v2-list-item-height);app/client/src/components/editorComponents/Debugger/StateInspector/StateInspector.tsx (1)
53-65: Consider simplifying the group mapping.The group mapping could be simplified by removing the empty className property since it's not being used.
groups={filteredItemGroups.map((item) => ({ groupTitle: item.group, items: item.items, - className: "", }))}app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx (1)
37-39: Consider using a callback for setState.Initialize state using a function to avoid unnecessary calculations during component initialization.
-const [visibleItems, setVisibleItems] = React.useState( - visibleItemsCount || group.items.length, -); +const [visibleItems, setVisibleItems] = React.useState(() => + visibleItemsCount || group.items.length +);app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx (2)
127-153: Remove redundant empty className property.The empty className property at line 150 is redundant and can be removed.
return { groupTitle: key, items: value.map((data) => ({ // ... other properties })), - className: "", };
133-149: Consider extracting the item mapping logic.The item mapping logic is complex and could be extracted into a separate function for better maintainability.
+const mapDatasourceToListItem = (data: any, dsUsageMap: any, groupedPlugins: any, currentSelectedDatasource: string, goToDatasource: Function) => ({ + id: data.id, + title: data.name, + startIcon: ( + <DatasourceIcon + src={getAssetUrl(groupedPlugins[data.pluginId].iconLocation)} + /> + ), + description: get(dsUsageMap, data.id, ""), + descriptionType: "block", + className: "t--datasource", + isSelected: currentSelectedDatasource === data.id, + onClick: () => goToDatasource(data.id), +}); items: value.map((data) => - return { - id: data.id, - // ... rest of the properties - } + mapDatasourceToListItem(data, dsUsageMap, groupedPlugins, currentSelectedDatasource, goToDatasource) ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/packages/design-system/ads/src/List/List.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/List/List.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts(1 hunks)app/client/src/ce/selectors/entitiesSelector.ts(1 hunks)app/client/src/components/editorComponents/Debugger/StateInspector/StateInspector.tsx(2 hunks)app/client/src/pages/Editor/Explorer/ExplorerIcons.tsx(1 hunks)app/client/src/pages/Editor/IDE/LeftPane/DataSidePane.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ce/selectors/entitiesSelector.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (3)
app/client/packages/design-system/ads/src/List/List.tsx (1)
121-121: LGTM! Good use of data attribute for stylingThe data attribute provides a clean way to apply conditional styles based on the description type.
app/client/packages/design-system/ads/src/List/List.styles.tsx (1)
98-100: Verify height accommodation for block descriptions.The increased max-height of 54px for block descriptions needs verification to ensure it adequately accommodates:
- Multi-line descriptions
- Various font sizes (especially in lg size mode)
- Line height and padding combinations
✅ Verification successful
Block description height of 54px is correctly configured
The max-height is sufficient as block descriptions are intentionally constrained to a single line with text overflow handling. This works for all font sizes, including the large size mode.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of block descriptions to verify height requirements ast-grep --pattern 'StyledListItem $props isBlockDescription={true}'Length of output: 70
Script:
#!/bin/bash # Search for List component usage with block descriptions rg -U "isBlockDescription.*?true" -g "*.{tsx,jsx}" # Search for style definitions related to List and block descriptions rg "data-isblockdescription" -g "*.{tsx,jsx,ts,css,scss}" # Search for font-size and line-height definitions in the List styles cat app/client/packages/design-system/ads/src/List/List.styles.tsx # Look for any height-related properties in List component rg "height|lineHeight" app/client/packages/design-system/ads/src/List/Length of output: 5496
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.types.ts (1)
19-19: LGTM! Clean type definition addition.The optional
visibleItemsCountproperty is well-typed and maintains backward compatibility.
...ackages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.tsx
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12744729296. |
|
Deploy-Preview-URL: https://ce-38621.dp.appsmith.com |
…o chore/replace-group-list-implementation
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12745692622. |
|
Deploy-Preview-URL: https://ce-38621.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12746282278. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/List/List.stories.tsx (2)
214-215: Use a more realistic example for the overflow title.Consider using a natural text example that better represents real-world scenarios, such as "This is a very long action item name that demonstrates text overflow behavior".
- title: - "Action_item_1_with_a_very_long_name_that_should_show_ellipsis_in_the_same_line", + title: + "This is a very long action item name that demonstrates text overflow behavior",
219-231: Enhance the block description example.The current description "block" doesn't effectively demonstrate the block description feature. Consider using a more meaningful example.
startIcon: <Icon name="file-list-2-line" size={"md"} />, title: "Action item 1", - description: "block", + description: "This is a detailed description that appears below the title, demonstrating the block layout.", descriptionType: "block",
📜 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(1 hunks)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)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/ads/src/List/List.tsx
- app/client/packages/design-system/ads/src/List/List.styles.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (1)
app/client/packages/design-system/ads/src/List/List.stories.tsx (1)
232-242: LGTM! Well-structured story definition.The story effectively demonstrates the block description variant without an icon, using clear and meaningful examples.
|
Deploy-Preview-URL: https://ce-38621.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12747815044. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx (1)
22-27: Consider using story controls forvisibleItems.Instead of hardcoding the value
5, consider:
- Using story controls to allow dynamic testing of different values
- Importing the same constant used by the application
- visibleItems={5} + visibleItems={args.visibleItems}And add to the story's argTypes:
argTypes: { visibleItems: { control: { type: 'number', min: 1 }, defaultValue: 5, }, },Also applies to: 91-91
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityGroupsList/EntityGroupsList.stories.tsx(2 hunks)app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx(2 hunks)app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx(2 hunks)app/client/src/pages/Editor/IDE/RightPane/components/CreateNewQueryModal.tsx(2 hunks)app/client/src/pages/Editor/IDE/constants.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/IDE/constants.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (3)
app/client/src/pages/Editor/IDE/RightPane/components/CreateNewQueryModal.tsx (1)
40-44: LGTM! Good UX improvement.The addition of
visibleItemsprop helps maintain consistency in list item visibility across the application.app/client/src/pages/Editor/IDE/EditorPane/Query/Add.tsx (1)
63-67: LGTM! Consistent with modal implementation.The
visibleItemsprop implementation aligns with the pattern used inCreateNewQueryModal.app/client/src/pages/Editor/IDE/EditorPane/JS/Add.tsx (1)
102-106: LGTM! Maintains consistency across components.The implementation follows the same pattern as Query and Modal components, ensuring a consistent user experience.
|
Deploy-Preview-URL: https://ce-38621.dp.appsmith.com |
ankitakinger
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12755373216. |
|
Deploy-Preview-URL: https://ce-38621.dp.appsmith.com |
ankitakinger
added
ok-to-test
Description
Replacing the entity group list component on state inspector and data side pane with new ADS component
Fixes #38290
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12763738469
Commit: a4b7172
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Tue, 14 Jan 2025 09:50:48 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates
EntityGroupsListcomponent for better maintainability.ListItemcomponent to showcase various configurations.