chore: Extracting out common logic between two ADS components to move it to one #38885
Conversation
WalkthroughThe pull request introduces significant refactoring 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (11)
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: 1
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (1)
60-60: Consider type safety for rest props.While spreading rest props is clean, consider explicitly typing which props are allowed to be spread to
ListItem.app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts (1)
13-16: Enhanced flexibility with optional props
The new optional props (canEdit,isFixedWidth,textKind,gap) provide finer control over behavior and appearance. Adding brief documentation could further clarify usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(1 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (18)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.stories.tsx (2)
25-33: LGTM! Props updated to match new component API.The story configuration properly demonstrates the new component API with appropriate test values for all props.
9-9: Verify icon size change impact on visual consistency.The icon size has been changed from "md" to "sm". Please ensure this change maintains visual consistency with other components using this icon.
✅ Verification successful
Icon size change is consistent with other story components
The "sm" size is consistently used across story components, while "md" size is used in the main application - this appears to be an intentional design pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of js-yellow icon to verify size consistency rg -l 'name="js-yellow"' | xargs rg 'size="[^"]+"' -A 1 -B 1Length of output: 3767
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.tsx (3)
5-5: LGTM: Clean import restructuring and prop destructuring.The changes simplify the component's interface by:
- Importing the new consolidated
EditableEntityNamecomponent- Using a more semantic
startIconprop nameAlso applies to: 17-17
38-47: LGTM: Comprehensive dependency array.All props used within the memoized component are correctly included in the dependencies array.
24-36: Verify prop types alignment with EditableEntityName component.The implementation looks clean, but we should ensure all required props are properly typed in the imported component.
✅ Verification successful
Props are correctly typed and implemented
All required and optional props in the EntityItem component match the EditableEntityName interface definition. No type misalignment found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for EditableEntityName prop types definition ast-grep --pattern 'interface $name { $$$ }' app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.tsLength of output: 147
Script:
#!/bin/bash # Find EditableEntityName related files fd EditableEntityNameLength of output: 490
Script:
#!/bin/bash # Examine the prop types and component implementation echo "=== EditableEntityName.types.ts ===" cat app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts echo -e "\n=== EditableEntityName.tsx ===" cat app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsxLength of output: 2683
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (5)
10-23: Use consistent prop defaults and ensure type safety
A default gap value is provided, which helps maintain consistent spacing without requiring additional checks. Also, destructuring props at the top keeps the component clean and readable.
25-25: Confirm behavior when edit is disallowed
inEditModeis forced tofalseifcanEditis unset, even ifisEditingistrue. This might be intentional; verify this logic aligns with your UX requirements.
33-39: Good use of theuseEditableTexthook
The hook usage is clean and properly passes necessary parameters. Keep an eye on future validations or edge cases (e.g., empty names).
41-48: Spinner usage improves clarity
Leveraging a loading spinner in place ofstartIconwhenisLoadingistrueis straightforward and keeps the UI consistent.
61-75: Clean layout with appropriate data attributes
The final rendering with tooltip for errors is neatly structured. The use ofdata-attributes forisEditingandisFixedWidthis a good practice for style or testing hooks.app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.types.ts (1)
5-5: Property rename maintains clarity
Changing fromicontostartIconclarifies intent and aligns with updates in related components.app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts (3)
2-2: CentralizedTextKindimport is consistent
Importing theTextKindtype ensures consistency across components using text variations.
5-5: Renaming prop tostartIcon
This renaming matches the new component approach for specifying icons on the left side.
10-10: Improved event naming withonEditComplete
The new callback name is more descriptive and avoids confusion about exit conditions.app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.stories.tsx (1)
14-14: LGTM! Icon size and prop name changes look good.The changes align well with the component refactoring:
- Simplified icon size to "sm"
- Updated to use the new
startIconpropAlso applies to: 25-25
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.styles.ts (2)
4-12: LGTM! Root component improvements enhance flexibility.The changes to Root component improve layout control:
- Dynamic gap through props
- Better flex behavior
- Proper width constraints
25-34: LGTM! Well-handled text overflow states.Good implementation of text overflow handling:
- Fixed width mode with ellipsis
- Proper overflow behavior during editing
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.tsx (1)
20-20: LGTM! Prop updates improve API clarity.The changes maintain functionality while improving the API:
- Consistent use of
startIcon- More descriptive
onEditCompletecallback nameAlso applies to: 44-44, 46-46
...ent/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.styles.ts
Outdated
Show resolved
Hide resolved
...t/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.tsx
Outdated
Show resolved
Hide resolved
...ient/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts
Outdated
Show resolved
Hide resolved
...ient/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.types.ts
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Outdated
Show resolved
Hide resolved
...ent/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.styles.ts
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Show resolved
Hide resolved
…o chore/common-editable-entity-name
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13030702276. |
|
Deploy-Preview-URL: https://ce-38885.dp.appsmith.com |
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Show resolved
Hide resolved
...es/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.stories.tsx
Show resolved
Hide resolved
...ent/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.styles.ts
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx
Outdated
Show resolved
Hide resolved
…o chore/common-editable-entity-name
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (1)
57-60: Consider moving styles to styled-componentsInline styles could be moved to styled-components for consistency.
- style: { - backgroundColor: "var(--ads-v2-color-bg)", - paddingTop: "4px", - paddingBottom: "4px", - top: "-5px", - },Consider adding these styles to Styled.Text component.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.types.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.styles.ts(1 hunks)app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx(2 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/packages/design-system/ads/src/Templates/EntityExplorer/EntityItem/EntityItem.styles.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.styles.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- 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: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (7)
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.tsx (2)
Line range hint
12-21: Clarify the relationship betweencanEditandisEditablepropsThe component has two props controlling edit functionality:
isEditableaffects double-click behavior whilecanEditis passed to child component. This could lead to inconsistent states.Consider consolidating these props or documenting their distinct purposes.
Line range hint
41-50: LGTM!Props are correctly passed to EditableEntityName component.
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.stories.tsx (2)
13-15: LGTM!Icon size change to "sm" aligns with the design system's tab specifications.
26-32: Verify edit mode functionality and validation requirementsTwo concerns:
- Previous review noted edit mode stopped working with
canEdit- Validation requirement reduced from 3 to 1 characters
Please confirm:
- Edit mode works with double-click
- 1-character minimum aligns with business requirements
app/client/packages/design-system/ads/src/Templates/EditableEntityName/EditableEntityName.tsx (2)
25-25: LGTM!Clean implementation of edit mode logic using canEdit prop.
41-48: LGTM!Well-structured loading state handling with proper spinner size alignment.
app/client/packages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.types.ts (1)
14-14: Clarify the distinction betweencanEditandisEditableprops.The interface now has two seemingly similar boolean flags for editing:
isEditableandcanEdit. This could lead to confusion for component consumers.Could you clarify:
- The difference between these props
- Whether they could be consolidated into a single prop
- The scenarios where both props might be needed
...kages/design-system/ads/src/Templates/EditableDismissibleTab/EditableDismissibleTab.types.ts
Outdated
Show resolved
Hide resolved
Description
Extracting out common logic between two ADS components to move it to one.
Fixes #38853
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13067939918
Commit: dd17cf1
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 31 Jan 2025 08:26:34 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
EditableEntityNamecomponent with new properties:canEditto control editabilityisFixedWidthfor width behaviorsizeoption for component sizingcanEditproperty inEditableDismissibleTabfor better editing controlBug Fixes
DataSidePaneStyle