fix: refactor wds menu and related components, widgets #33703
Conversation
KelvinOm
requested review from
a team,
riodeuno,
jsartisan and
marks0351
and removed request for
a team
WalkthroughWalkthroughThis update introduces significant enhancements and refactoring across various components of the design system. Key changes include the addition of new props to the Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (4)
Files selected for processing (57)
Files not processed due to max files limit (1)
Files not summarized due to errors (3)
Files not reviewed due to errors (3)
Files skipped from review due to trivial changes (18)
Additional comments not posted (57)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| SpectrumFieldProps, | ||
| "showErrorIcon" | "labelPosition" | "labelAlign" | keyof StyleProps | ||
| | "contextualHelp" |
There was a problem hiding this comment.
Types are broken due to package updates.
| @@ -64,7 +64,7 @@ const _Icon = (props: IconProps, ref: Ref<SVGSVGElement>) => { | |||
| ref={ref} | |||
| {...rest} | |||
| > | |||
| <Icon {...props} /> | |||
There was a problem hiding this comment.
Fix for these errors
| @@ -32,4 +32,13 @@ | |||
| a:not(.docs-story a) { | |||
| color: var(--color-fg-accent) !important; | |||
| } | |||
|
|
|||
| .css-qa4clq :where(div:not(.sb-anchor, .sb-unstyled, .sb-unstyled div)), | |||
There was a problem hiding this comment.
Fixing the typography in Storybrook for menu items
KelvinOm
added
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9210338526. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (3)
app/client/packages/design-system/widgets/src/components/Menu/stories/Menu.stories.tsx (1)
49-49: Consider providing visual feedback or logging instead of usingalertfor action feedback in production code.app/client/src/widgets/wds/WDSInlineButtonsWidget/widget/index.tsx (1)
[!TIP]
Codebase VerificationThe
WDSInlineButtonsWidgetis a class-based component extendingBaseWidget, but it does not implementcomponentDidMountorcomponentWillUnmount. Additionally, there are nouseEffecthooks, indicating that side effects and lifecycle management might not be handled properly.
- File:
app/client/src/widgets/wds/WDSInlineButtonsWidget/widget/index.tsx- Class:
WDSInlineButtonsWidgetAnalysis chain
Line range hint
1-1: Review the widget's lifecycle methods to ensure they are correctly implemented and do not introduce memory leaks or performance issues.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct implementation of lifecycle methods in WDSInlineButtonsWidget. # Test: Check for correct implementation of lifecycle methods. rg --type typescript --context 5 'componentDidMount|componentWillUnmount' app/client/src/widgets/wds/WDSInlineButtonsWidget/widget/index.tsxLength of output: 177
Script:
#!/bin/bash # Description: Verify correct implementation of lifecycle methods in WDSInlineButtonsWidget. # Test: Check for correct implementation of lifecycle methods. rg --type ts --context 5 'componentDidMount|componentWillUnmount' app/client/src/widgets/wds/WDSInlineButtonsWidget/widget/index.tsxLength of output: 134
Script:
#!/bin/bash # Description: Verify correct implementation of lifecycle methods or hooks in WDSInlineButtonsWidget. # Test: Check for useEffect hook usage. rg --type ts --context 5 'useEffect' app/client/src/widgets/wds/WDSInlineButtonsWidget/widget/index.tsxLength of output: 103
Script:
#!/bin/bash # Description: Verify the structure and side effect management in WDSInlineButtonsWidget. # Test: Check for the component definition and any hooks or methods used. rg --type ts --context 5 'function|const' app/client/src/widgets/wds/WDSInlineButtonsWidget/widget/index.tsxLength of output: 718
app/client/packages/design-system/headless/src/components/Field/src/Field.tsx (1)
7-29: Ensure new properties inFieldPropsare well-documented.Adding detailed comments for each property in
FieldPropswould improve maintainability and developer understanding, especially for properties likeelementTypeandnecessityIndicator.
| .inlineButtons { | ||
| display: flex; | ||
| width: 100%; | ||
| gap: var(--inner-spacing-2); | ||
|
|
||
| & :is([data-button]) { | ||
| min-inline-size: fit-content; | ||
| /* | ||
| We use !important here to be sure that button width and the logic of useInlineButtons hook will not be changed from the outside | ||
| */ | ||
| min-inline-size: fit-content !important; |
There was a problem hiding this comment.
Consider alternatives to using !important in CSS to allow easier style customization and maintenance. However, if it's necessary for ensuring consistency in UI behavior, the current approach may be justified.
Also applies to: 22-22
| necessityIndicator={ | ||
| !Boolean(isReadOnly) ? necessityIndicator : undefined | ||
| } |
There was a problem hiding this comment.
Refactor to simplify necessity indicator logic.
The current implementation of the necessity indicator is a bit complex. Consider simplifying it as follows:
- !Boolean(isReadOnly) ? necessityIndicator : undefined
+ isReadOnly ? undefined : necessityIndicatorThis change makes the condition more readable by directly checking the isReadOnly state.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| necessityIndicator={ | |
| !Boolean(isReadOnly) ? necessityIndicator : undefined | |
| } | |
| necessityIndicator={ | |
| isReadOnly ? undefined : necessityIndicator | |
| } |
| icon: "thumb-up", | ||
| isSeparator, |
There was a problem hiding this comment.
Consider making the icon property configurable to enhance flexibility.
- icon: "thumb-up",
+ icon: isSeparator ? undefined : "thumb-up", // or pass an icon parameterCommittable suggestion was skipped due low confidence.
| <ToolbarButtons | ||
| items={[ | ||
| { id: 1, label: "Fast food" }, | ||
| { id: 2, label: "Salads" }, | ||
| { id: 3, label: "Salads" }, | ||
| { id: 4, label: "Sauces" }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Ensure unique IDs for ToolbarButtons items.
The ToolbarButtons component is used with non-unique IDs for items (two items with ID 3). This could lead to unexpected behavior or accessibility issues. Consider ensuring unique IDs for each item:
- { id: 3, label: "Salads" },
+ { id: 3, label: "Desserts" },Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| <ToolbarButtons | |
| items={[ | |
| { id: 1, label: "Fast food" }, | |
| { id: 2, label: "Salads" }, | |
| { id: 3, label: "Salads" }, | |
| { id: 4, label: "Sauces" }, | |
| ]} | |
| /> | |
| <ToolbarButtons | |
| items={[ | |
| { id: 1, label: "Fast food" }, | |
| { id: 2, label: "Salads" }, | |
| { id: 3, label: "Desserts" }, | |
| { id: 4, label: "Sauces" }, | |
| ]} | |
| /> |
| let children = [...state.collection]; | ||
| const menuChildren = (props.items as ToolbarButtonsItem[]).slice( | ||
| visibleItems, | ||
| ); | ||
| children = children.slice(0, visibleItems); | ||
|
|
||
| return ( | ||
| <FocusScope> | ||
| <div | ||
| className={styles.toolbarButtons} | ||
| data-alignment={alignment} | ||
| data-density={Boolean(density) ? density : undefined} | ||
| ref={domRef} | ||
| {...toolbarButtonsProps} | ||
| > | ||
| {children.map((item) => { | ||
| if (Boolean(item.props.isSeparator)) { | ||
| return <div data-separator="" key={item.key} role="separator" />; | ||
| } | ||
|
|
||
| return ( | ||
| <ToolbarButton | ||
| color={color} | ||
| icon={item.props.icon} | ||
| iconPosition={item.props.iconPosition} | ||
| isDisabled={ | ||
| Boolean(state.disabledKeys.has(item.key)) || | ||
| Boolean(item.props.isDisabled) || | ||
| isDisabled | ||
| } | ||
| isLoading={item.props.isLoading} | ||
| item={item} | ||
| key={item.key} | ||
| onPress={() => onAction?.(item.key)} | ||
| size={Boolean(size) ? size : undefined} | ||
| state={state} | ||
| variant={variant} | ||
| /> | ||
| ); | ||
| })} | ||
| {menuChildren?.length > 0 && ( | ||
| <MenuTrigger> | ||
| <Button | ||
| color={color} | ||
| data-action-group-menu | ||
| icon="dots" | ||
| isDisabled={isDisabled} | ||
| variant={variant} | ||
| /> | ||
| <Menu {...props} items={menuChildren} /> | ||
| </MenuTrigger> | ||
| )} |
There was a problem hiding this comment.
Optimize rendering logic in ToolbarButtons.
The rendering logic in ToolbarButtons can be optimized to avoid unnecessary re-renders and improve performance. Consider using React.memo or React.useMemo for parts of the component that depend on specific props:
+ const MemoizedToolbarButton = React.memo(ToolbarButton);
- <ToolbarButton ... />
+ <MemoizedToolbarButton ... />This change helps prevent unnecessary re-renders when the props to ToolbarButton have not changed.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let children = [...state.collection]; | |
| const menuChildren = (props.items as ToolbarButtonsItem[]).slice( | |
| visibleItems, | |
| ); | |
| children = children.slice(0, visibleItems); | |
| return ( | |
| <FocusScope> | |
| <div | |
| className={styles.toolbarButtons} | |
| data-alignment={alignment} | |
| data-density={Boolean(density) ? density : undefined} | |
| ref={domRef} | |
| {...toolbarButtonsProps} | |
| > | |
| {children.map((item) => { | |
| if (Boolean(item.props.isSeparator)) { | |
| return <div data-separator="" key={item.key} role="separator" />; | |
| } | |
| return ( | |
| <ToolbarButton | |
| color={color} | |
| icon={item.props.icon} | |
| iconPosition={item.props.iconPosition} | |
| isDisabled={ | |
| Boolean(state.disabledKeys.has(item.key)) || | |
| Boolean(item.props.isDisabled) || | |
| isDisabled | |
| } | |
| isLoading={item.props.isLoading} | |
| item={item} | |
| key={item.key} | |
| onPress={() => onAction?.(item.key)} | |
| size={Boolean(size) ? size : undefined} | |
| state={state} | |
| variant={variant} | |
| /> | |
| ); | |
| })} | |
| {menuChildren?.length > 0 && ( | |
| <MenuTrigger> | |
| <Button | |
| color={color} | |
| data-action-group-menu | |
| icon="dots" | |
| isDisabled={isDisabled} | |
| variant={variant} | |
| /> | |
| <Menu {...props} items={menuChildren} /> | |
| </MenuTrigger> | |
| )} | |
| let children = [...state.collection]; | |
| const menuChildren = (props.items as ToolbarButtonsItem[]).slice( | |
| visibleItems, | |
| ); | |
| children = children.slice(0, visibleItems); | |
| + const MemoizedToolbarButton = React.memo(ToolbarButton); | |
| return ( | |
| <FocusScope> | |
| <div | |
| className={styles.toolbarButtons} | |
| data-alignment={alignment} | |
| data-density={Boolean(density) ? density : undefined} | |
| ref={domRef} | |
| {...toolbarButtonsProps} | |
| > | |
| {children.map((item) => { | |
| if (Boolean(item.props.isSeparator)) { | |
| return <div data-separator="" key={item.key} role="separator" />; | |
| } | |
| return ( | |
| <MemoizedToolbarButton | |
| color={color} | |
| icon={item.props.icon} | |
| iconPosition={item.props.iconPosition} | |
| isDisabled={ | |
| Boolean(state.disabledKeys.has(item.key)) || | |
| Boolean(item.props.isDisabled) || | |
| isDisabled | |
| } | |
| isLoading={item.props.isLoading} | |
| item={item} | |
| key={item.key} | |
| onPress={() => onAction?.(item.key)} | |
| size={Boolean(size) ? size : undefined} | |
| state={state} | |
| variant={variant} | |
| /> | |
| ); | |
| })} | |
| {menuChildren?.length > 0 && ( | |
| <MenuTrigger> | |
| <Button | |
| color={color} | |
| data-action-group-menu | |
| icon="dots" | |
| isDisabled={isDisabled} | |
| variant={variant} | |
| /> | |
| <Menu {...props} items={menuChildren} /> | |
| </MenuTrigger> | |
| )} |
| if (buttonsList[key].onClick) { | ||
| setLoadingButtonIds([...loadingButtonIds, key as string]); | ||
|
|
||
| props.onButtonClick(props.buttonsList[key].onClick, () => | ||
| onActionComplete(props.buttonsList[key]), | ||
| onButtonClick(buttonsList[key].onClick, () => | ||
| onActionComplete(buttonsList[key]), |
There was a problem hiding this comment.
Tip
Codebase Verification
The search did not find any explicit error handling mechanisms such as try-catch blocks or error callbacks in the specified file. This suggests that there might be a lack of error handling for the onClick action, which could indeed cause the UI to become unresponsive if an error occurs.
- Ensure that the
onClickaction has proper error handling to prevent the UI from becoming unresponsive in case of failures.
Analysis chain
Review the error handling for the onClick action to ensure that failures in the action do not cause the UI to become unresponsive.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Check for error handling in onClick action.
rg --type typescript --context 5 'onClick' app/client/src/widgets/wds/WDSInlineButtonsWidget/component/index.tsx
Length of output: 147
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Check for error handling in onClick action.
rg --type ts --context 5 'onClick' app/client/src/widgets/wds/WDSInlineButtonsWidget/component/index.tsx
Length of output: 789
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Check for error handling mechanisms such as try-catch blocks or error callbacks in the same file.
rg --type ts --context 5 'try' app/client/src/widgets/wds/WDSInlineButtonsWidget/component/index.tsx
# Test: Check for error handling mechanisms such as error callbacks in the same file.
rg --type ts --context 5 'catch' app/client/src/widgets/wds/WDSInlineButtonsWidget/component/index.tsx
# Test: Check for error handling mechanisms such as error callbacks in the same file.
rg --type ts --context 5 'error' app/client/src/widgets/wds/WDSInlineButtonsWidget/component/index.tsx
Length of output: 306
| @@ -14,19 +14,21 @@ export const ToolbarButtonsComponent = ( | |||
| Array<ToolbarButtonsItemComponentProps["id"]> | |||
| >([]); | |||
|
|
|||
| const { alignment, buttonsList, color, onButtonClick, variant } = props; | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The code handles the sorting and disabling logic for buttonsList and loadingButtonIds. It filters visible buttons, sorts them, and manages disabled buttons and loading states. However, it does not explicitly handle edge cases like empty lists or all buttons being disabled.
- Ensure that
buttonsListis not empty before performing operations. - Handle cases where all buttons might be disabled.
Analysis chain
Ensure that the sorting and disabling logic correctly handles all edge cases, such as empty lists or all buttons being disabled.
Also applies to: 28-29
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that sorting and disabling logic handles edge cases.
# Test: Check for edge case handling in sorting and disabling logic.
rg --type typescript --context 5 'sortBy' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/index.tsx
Length of output: 147
Script:
#!/bin/bash
# Description: Verify that sorting and disabling logic handles edge cases.
# Test: Check for edge case handling in sorting and disabling logic.
rg --type ts --context 5 'sortBy' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/index.tsx
Length of output: 757
Script:
#!/bin/bash
# Description: Verify handling of edge cases for buttonsList and loadingButtonIds.
# Test: Check for edge case handling in buttonsList and loadingButtonIds.
rg --type ts --context 5 'buttonsList|loadingButtonIds' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/index.tsx
Length of output: 1407
| if (buttonsList[key].onClick) { | ||
| setLoadingButtonIds([...loadingButtonIds, key as string]); | ||
|
|
||
| props.onButtonClick(props.buttonsList[key].onClick, () => | ||
| onActionComplete(props.buttonsList[key]), | ||
| onButtonClick(buttonsList[key].onClick, () => | ||
| onActionComplete(buttonsList[key]), |
There was a problem hiding this comment.
Tip
Codebase Verification
The implementation of onButtonClick in WDSToolbarButtonsWidget does not include explicit error handling. This could potentially cause the UI to become unresponsive if the action fails.
- Consider adding error handling within the
onButtonClickmethod to ensure that any failures in the action do not cause the UI to become unresponsive.
Analysis chain
Review the error handling for the onClick action to ensure that failures in the action do not cause the UI to become unresponsive.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Check for error handling in onClick action.
rg --type typescript --context 5 'onClick' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/index.tsx
Length of output: 148
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Check for error handling in onClick action.
rg --type ts --context 5 'onClick' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/index.tsx
Length of output: 745
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Check for the definition and implementation of onButtonClick.
rg --type ts 'function onButtonClick' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/
# Test: Check for the definition and implementation of onActionComplete.
rg --type ts 'function onActionComplete' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/
Length of output: 206
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Search for the usage and definition of onButtonClick.
rg --type ts 'onButtonClick' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/
# Test: Search for the usage and definition of onActionComplete.
rg --type ts 'onActionComplete' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/
Length of output: 803
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Extract the full implementation of onActionComplete.
rg --type ts --context 10 'const onActionComplete' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/index.tsx
# Test: Extract the type definition of onButtonClick.
rg --type ts --context 10 'onButtonClick' app/client/src/widgets/wds/WDSToolbarButtonsWidget/component/types.ts
Length of output: 1380
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Search for the implementation of onButtonClick.
rg --type ts 'onButtonClick'
Length of output: 4055
Script:
#!/bin/bash
# Description: Verify error handling for onClick action.
# Test: Extract the implementation of onButtonClick in WDSToolbarButtonsWidget.
rg --type ts --context 10 'onButtonClick' app/client/src/widgets/wds/WDSToolbarButtonsWidget/widget/index.tsx
Length of output: 957
|
Deploy-Preview-URL: https://ce-33703.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
app/client/src/widgets/wds/WDSInlineButtonsWidget/config/defaultsConfig.ts (1)
26-26: Properly document the use ofisSeparatorfor clarity.Consider adding a comment here explaining the purpose and usage of
isSeparatorto improve code readability.app/client/src/widgets/wds/WDSToolbarButtonsWidget/config/defaultsConfig.ts (1)
48-48: Document the purpose ofisSeparatorin the context of ToolbarButtons.It would be beneficial to add a comment here explaining the use of
isSeparatorto enhance code readability and maintainability.app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (1)
59-66: Check for potential duplication of 'Salads' in theToolbarButtonsitems.Consider whether having two 'Salads' items is intentional or an oversight. If it's not required, you might want to remove the duplicate to avoid confusion.
| variant = "filled", | ||
| } = props; | ||
| const domRef = useDOMRef(ref); | ||
| const state = useListState({ ...props, suppressTextValueWarning: true }); |
There was a problem hiding this comment.
wont this create a new object everytime the widget renders?
There was a problem hiding this comment.
It shouldn't, as far as I can see from the code of this hook.
| domRef, | ||
| ); | ||
|
|
||
| const children = [...state.collection]; |
There was a problem hiding this comment.
why do we want to create a new array by spreading?
There was a problem hiding this comment.
Because collection is not an array and we need it to iterate through items.
|
@KelvinOm Looks like both @marks0351 and @jsartisan have review this. Is there anything you'd like for me to check out? If not, I've removed myself from the reviewers list for now. |
@riodeuno Two reviewers are enough. You can skip this. Thanks. |
Description
react-aria-componentsinstead. The menu component now has the ability to use submenus and icons for items.ActionGrouptoToolbarButtons andButtonGrouptoInlineButtons`ToolbarButtons,Menu,InlineButtons,Selectconsistent.ToolbarButtonsandInlineButtonsand related widgets.Fixes:
#32299
#32109
Automation
/ok-to-test tags="@tag.Anvil"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9210315418
Commit: b11c002
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?