chore: Housekeeping items in List and Entity Item ADS #38537
Conversation
WalkthroughThis pull request introduces refinements to the design system's List and EntityItem components, focusing on event handling, type definitions, and component identification. The changes primarily involve updating event callbacks, adding test IDs, and introducing new properties like Changes
Sequence DiagramsequenceDiagram
participant User
participant EntityItem
participant ListItem
participant EventHandler
User->>EntityItem: Interact
EntityItem->>ListItem: Trigger Event
ListItem->>EventHandler: Process Event
EventHandler-->>ListItem: Handle Callback
ListItem-->>EntityItem: Update State
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
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (2)
7-7: Consider renaming import to standard convention.The
clximport alias is less common. Consider using the standard nameclassnames.-import clx from "classnames"; +import classnames from "classnames";
81-84: Consider deriving data-testid from id for consistency.Currently, data-testid uses title while id uses a different value. Consider using id for both to maintain consistency.
- className={clx("t--entity-item", props.className)} + className={classnames("t--entity-item", props.className)} customTitleComponent={customTitle} - data-testid={`t--entity-item-${props.title}`} + data-testid={`t--entity-item-${props.id}`} id={"entity-" + props.id}app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx (1)
39-39: Consider adding id to all story variants.While the id is set in the Template, consider adding different ids in the story variants (InEditingMode, NoPermissionToEdit, etc.) to demonstrate different id values.
Example for story variants:
InEditingMode.args = { id: "editing-mode-item", title: "EntityName", // ... rest of the props };app/client/packages/design-system/ads/src/List/List.tsx (1)
103-105: Consider adding a comment explaining the stopPropagation.While the implementation is correct, it would be helpful to document why event propagation needs to be stopped here.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.test.tsx (1)
85-85: Add assertions for stopPropagation callsWhile the mock for
stopPropagationis correctly added, we should verify that it's being called. Add expectations like:expect(stopPropagationMock).toHaveBeenCalled();Example implementation:
const stopPropagationMock = jest.fn(); handleKeyUp({ key: "Enter", - stopPropagation: jest.fn(), + stopPropagation: stopPropagationMock, } as unknown as React.KeyboardEvent<HTMLInputElement>); +expect(stopPropagationMock).toHaveBeenCalled();Also applies to: 117-117, 139-142, 163-166
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
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/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.test.tsx(4 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (8)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.types.ts (1)
8-9: LGTM! Well-documented type addition.The new
idproperty is properly typed and documented.app/client/packages/design-system/ads/src/List/List.types.tsx (1)
2-2: LGTM! Improved type safety for event handling.The addition of MouseEvent type and updated onClick signature properly type the event parameter, aligning with React's event system.
Also applies to: 14-14
app/client/packages/design-system/ads/src/List/List.tsx (4)
40-57: Improved tooltip performance with event-driven approach.Good refactor from ref-based DOM queries to direct event handling. The useEventCallback ensures stable event handlers across renders.
87-93: LGTM! Proper event handling implementation.Good addition of stopPropagation() and proper event typing. The handler correctly checks for disabled state before invoking the callback.
95-101: LGTM! Consistent event handling pattern.The double click handler follows the same pattern as onClick, maintaining code consistency.
113-113: LGTM! Proper id propagation.The id prop is correctly passed to the DOM element for accessibility and testing purposes.
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.ts (1)
77-78: Verify the necessity of stopping event propagationAdding
stopPropagation()will prevent Enter/Escape key events from reaching parent components. Ensure this doesn't break any parent component's keyboard event handling that might be relying on these events.✅ Verification successful
Event propagation change is safe to implement
The
stopPropagation()addition is safe as the parent component (EntityItem) only forwards the hook's keyboard handler without adding its own event handling logic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for parent components that might be listening for Enter/Escape key events rg -B 2 -A 2 "onKeyUp|onKeyDown|onKeyPress" app/client/packages/design-system/ads/src/Templates/EntityExplorer/Length of output: 692
Script:
#!/bin/bash # Get the implementation of handleKeyUp in EntityItem ast-grep --pattern 'const handleKeyUp = $_' app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsxLength of output: 138
Script:
#!/bin/bash # Search for handleKeyUp with more context rg -B 5 -A 5 "handleKeyUp" app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsxLength of output: 672
app/client/packages/design-system/ads/src/Templates/EntityExplorer/Editable/useEditableText.test.tsx (1)
85-85: LGTM! Test coverage is completeThe stopPropagation mock has been consistently added to all keyboard event test cases.
Also applies to: 117-117, 139-142, 163-166
Description
General housekeeping items to ADS ListItem, Editable and Entity Item. In prep for usage of EntityListTree we found these items missing. PR #38493
Automation
/ok-to-test tags="@tag.Sanity, @tag.IDE"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12668515385
Commit: 0fac0e7
Cypress dashboard.
Tags:
@tag.Sanity, @tag.IDESpec:
Wed, 08 Jan 2025 11:23:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
The updates focus on improving component interactions, event handling, and testing infrastructure across various design system components.