chore: remove headless radio and use react-aria component instead #34312
Conversation
github-actions
bot
added
Anvil Pod
WalkthroughThe updates encompass various enhancements across several components within the design system module. Key changes include re-exporting components, introducing new props, adjusting styles, and removing outdated elements. Notably, this includes updates to Checkbox, Label, RadioGroup, and the addition of the ErrorMessage component, leading to better integration, functionality, and user experience within the system. Changes
Sequence Diagram(s)The changes are too varied and simple to warrant a unified sequence diagram. Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
skip-changelog
KelvinOm
added
ok-to-test
github-actions
bot
added
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9567395642. |
github-actions
bot
added
Bug
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between f718bfb and 62d1153caa8bf03d827f88593c9dfaf3121091ee.
Files selected for processing (20)
- app/client/packages/design-system/headless/src/index.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/ErrorMessage.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/index.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/styles.module.css (1 hunks)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/types.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/Label/src/Label.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/Label/src/styles.module.css (1 hunks)
- app/client/packages/design-system/widgets/src/components/Label/src/types.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/RadioGroup.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/styles.module.css (6 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/stories/RadioGroup.stories.tsx (4 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/tests/RadioGroup.test.tsx (5 hunks)
- app/client/packages/design-system/widgets/src/components/ToggleGroup/chromatic/Group.chromatic.stories.tsx (2 hunks)
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/ToggleGroup.tsx (3 hunks)
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/styles.module.css (2 hunks)
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/types.ts (1 hunks)
- app/client/packages/design-system/widgets/src/index.ts (2 hunks)
- app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (2 hunks)
- app/client/src/widgets/wds/WDSRadioGroupWidget/widget/index.tsx (3 hunks)
- app/client/src/widgets/wds/WDSRadioGroupWidget/widget/types.ts (1 hunks)
Files not reviewed due to errors (5)
- app/client/packages/design-system/widgets/src/components/Label/src/Label.tsx (no review received)
- app/client/packages/design-system/widgets/src/components/RadioGroup/stories/RadioGroup.stories.tsx (no review received)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/RadioGroup.tsx (no review received)
- app/client/packages/design-system/widgets/src/components/RadioGroup/tests/RadioGroup.test.tsx (no review received)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/styles.module.css (no review received)
Files skipped from review due to trivial changes (6)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/index.ts
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/types.ts
- app/client/packages/design-system/widgets/src/components/Label/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/types.ts
Additional context used
Biome
app/client/packages/design-system/widgets/src/components/ErrorMessage/src/ErrorMessage.tsx
[error] 9-9: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancallapp/client/packages/design-system/widgets/src/components/Label/src/Label.tsx
[error] 12-12: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
[error] 12-12: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
Additional comments not posted (12)
app/client/packages/design-system/widgets/src/components/Label/src/types.ts (1)
7-7: Addition ofisDisabledproperty toLabelPropsis a good enhancement for conditional rendering based on the disabled state.app/client/packages/design-system/headless/src/index.ts (1)
Line range hint
1-10: The removal of theRadiocomponent export is consistent with the PR's objectives to replace it withreact-ariacomponents. Ensure all dependent modules have been updated accordingly.Verification successful
The removal of the
Radiocomponent export fromindex.tsis consistent with the PR's objectives to replace it withreact-ariacomponents. The search confirmed that no other files are importing the removedRadiocomponent.
- No imports found for the
Radiocomponent in the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no other files are importing the removed Radio component. # Test: Search for imports of the Radio component. Expect: No occurrences. rg --type python --files-with-matches 'from "./components/Radio"'Length of output: 98
Script:
#!/bin/bash # Description: Verify that no other files are importing the removed Radio component. # Test: Search for imports of the Radio component. Expect: No occurrences. rg --type js,ts --files-with-matches 'from "./components/Radio"' || echo "No imports found for Radio component"Length of output: 179
Script:
#!/bin/bash # Description: Verify that no other files are importing the removed Radio component. # Test: Search for imports of the Radio component. Expect: No occurrences. rg --type-add 'ts:*.ts' --type-add 'js:*.js' --files-with-matches 'from "./components/Radio"' || echo "No imports found for Radio component"Length of output: 178
app/client/src/widgets/wds/WDSRadioGroupWidget/widget/types.ts (1)
5-5: Update ofoptionsproperty to useRadioGroupProps["items"]aligns with the refactoring of theRadioGroupcomponent to enhance modularity. Ensure this change is well integrated in the widget implementation.app/client/packages/design-system/widgets/src/index.ts (2)
25-25: Correctly added export forErrorMessagecomponent.
29-29: Correctly added export forhooks.app/client/packages/design-system/widgets/src/components/ToggleGroup/chromatic/Group.chromatic.stories.tsx (1)
30-30: Correct usage ofitemsprop inRadioGroupandToggleGroupcomponents within the story configuration.app/client/packages/design-system/widgets/src/components/ToggleGroup/src/ToggleGroup.tsx (3)
3-8: Correctly added necessary imports forErrorMessage,Label,Flex, anduseGroupOrientation.
21-21: Proper handling and destructuring of new props such asisDisabled,isRequired,items, andlabel.
37-37: Updated rendering logic correctly integrates new props and conditionally renders components based on these props.Also applies to: 41-52, 56-56
app/client/src/widgets/wds/WDSRadioGroupWidget/widget/index.tsx (2)
9-9: The updated import statement correctly reflects the removal of theRadiocomponent, aligning with the PR objectives.
Line range hint
125-138: ThegetWidgetViewmethod has been updated to pass theitemsprop to theRadioGroupcomponent, reflecting the changes in its API. Ensure that thelabelTooltipis appropriately used ascontextualHelpand that the validation logic correctly sets theisInvalidstate.app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (1)
107-127: TheRadioGroupcomponent's usage here correctly implements the newitemsprop structure. Each item is properly defined withvalueandlabel, ensuring clear and consistent options for the user.
|
Deploy-Preview-URL: https://ce-34312.dp.appsmith.com |
KelvinOm
force-pushed
the
fix/wds-radiobutton
branch
from
62d1153 to
a71853d
Compare
github-actions
bot
added
Bug
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 62d1153caa8bf03d827f88593c9dfaf3121091ee and a71853d.
Files selected for processing (22)
- app/client/packages/design-system/headless/src/index.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/ErrorMessage.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/index.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/styles.module.css (1 hunks)
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/types.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/Label/src/Label.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/Label/src/styles.module.css (1 hunks)
- app/client/packages/design-system/widgets/src/components/Label/src/types.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/chromatic/RadioGroup.chromatic.stories.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/RadioGroup.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/styles.module.css (6 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/stories/RadioGroup.stories.tsx (4 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/tests/RadioGroup.test.tsx (5 hunks)
- app/client/packages/design-system/widgets/src/components/ToggleGroup/chromatic/Group.chromatic.stories.tsx (2 hunks)
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/ToggleGroup.tsx (3 hunks)
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/styles.module.css (2 hunks)
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/types.ts (1 hunks)
- app/client/packages/design-system/widgets/src/index.ts (2 hunks)
- app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx (2 hunks)
- app/client/packages/storybook/src/components/DataAttrWrapper.tsx (2 hunks)
- app/client/src/widgets/wds/WDSRadioGroupWidget/widget/index.tsx (3 hunks)
- app/client/src/widgets/wds/WDSRadioGroupWidget/widget/types.ts (1 hunks)
Files skipped from review as they are similar to previous changes (18)
- app/client/packages/design-system/headless/src/index.ts
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/index.ts
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/ErrorMessage/src/types.ts
- app/client/packages/design-system/widgets/src/components/Label/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/Label/src/types.ts
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/RadioGroup.tsx
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/RadioGroup/stories/RadioGroup.stories.tsx
- app/client/packages/design-system/widgets/src/components/RadioGroup/tests/RadioGroup.test.tsx
- app/client/packages/design-system/widgets/src/components/ToggleGroup/chromatic/Group.chromatic.stories.tsx
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/ToggleGroup.tsx
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/styles.module.css
- app/client/packages/design-system/widgets/src/components/ToggleGroup/src/types.ts
- app/client/packages/design-system/widgets/src/index.ts
- app/client/packages/design-system/widgets/src/testing/ComplexForm.tsx
- app/client/src/widgets/wds/WDSRadioGroupWidget/widget/index.tsx
- app/client/src/widgets/wds/WDSRadioGroupWidget/widget/types.ts
Additional context used
Biome
app/client/packages/design-system/widgets/src/components/ErrorMessage/src/ErrorMessage.tsx
[error] 9-9: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancallapp/client/packages/design-system/widgets/src/components/Label/src/Label.tsx
[error] 12-12: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
[error] 12-12: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancallapp/client/packages/storybook/src/components/DataAttrWrapper.tsx
[error] 23-23: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
[error] 35-35: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
Additional comments not posted (4)
app/client/packages/design-system/widgets/src/components/ErrorMessage/src/ErrorMessage.tsx (1)
11-20: The error message rendering logic and properties are appropriately set.app/client/packages/design-system/widgets/src/components/Label/src/Label.tsx (1)
17-17: The handling of theisDisabledprop with thedata-disabledattribute is correctly implemented.app/client/packages/storybook/src/components/DataAttrWrapper.tsx (1)
6-6: The addition of the optionaltargetprop enhances the flexibility of theDataAttrWrappercomponent.app/client/packages/design-system/widgets/src/components/RadioGroup/chromatic/RadioGroup.chromatic.stories.tsx (1)
20-35: The story definitions for theRadioGroupcomponent are well-structured and provide a comprehensive demonstration of the component's states.
| export const ErrorMessage = (props: ErrorMessageProps) => { | ||
| const { text } = props; | ||
|
|
||
| if (!Boolean(text)) return null; |
There was a problem hiding this comment.
Simplify the conditional check to improve readability.
- if (!Boolean(text)) return null;
+ if (!text) return null;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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!Boolean(text)) return null; | |
| if (!text) return null; |
Tools
Biome
[error] 9-9: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
| const { className, contextualHelp, isDisabled, isRequired, text, ...rest } = | ||
| props; | ||
|
|
||
| if (!Boolean(text) && !Boolean(contextualHelp)) return null; |
There was a problem hiding this comment.
Simplify the conditional checks to improve code readability.
- if (!Boolean(text) && !Boolean(contextualHelp)) return null;
+ if (!text && !contextualHelp) return null;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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!Boolean(text) && !Boolean(contextualHelp)) return null; | |
| if (!text && !contextualHelp) return null; |
Tools
Biome
[error] 12-12: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
[error] 12-12: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
| @@ -19,7 +20,11 @@ export const DataAttrWrapper = (props: DataAttrWrapperProps) => { | |||
| Boolean(ref.current.setAttribute) && | |||
| typeof ref.current.setAttribute === "function" | |||
| ) { | |||
| ref.current.setAttribute(attr, ""); | |||
| if (Boolean(target)) { | |||
There was a problem hiding this comment.
Simplify the conditional checks to improve code readability.
- if (Boolean(target)) {
+ if (target) {
ref.current.querySelector(target).setAttribute(attr, "");
- if (Boolean(domNode)) domNode.setAttribute(attr, "");
+ if (domNode) domNode.setAttribute(attr, "");Also applies to: 35-35
Tools
Biome
[error] 23-23: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
app/client/packages/design-system/widgets/src/components/RadioGroup/src/RadioGroup.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/widgets/src/components/RadioGroup/src/styles.module.css
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
QQ: didn't we decide to remove toggle group ( I remember a thread on slack you asking about it )
There was a problem hiding this comment.
Nope, Taras said that we need it.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9578137232. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/packages/design-system/widgets/src/components/Checkbox/src/Checkbox.tsx (1)
Line range hint
28-28: Consider removing the redundantBooleancalls. They are unnecessary as the values are already being used in a boolean context, and this simplifies the code.- {Boolean(isIndeterminate) ? <Icon name="minus" /> : <Icon name="check" />} + {isIndeterminate ? <Icon name="minus" /> : <Icon name="check" />} - {Boolean(isRequired) && ( + {isRequired && (
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- app/client/packages/design-system/widgets/src/components/Checkbox/src/Checkbox.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/Checkbox/src/index.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/Checkbox/src/types.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/RadioGroup.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/index.ts (1 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/styles.module.css (5 hunks)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/types.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- app/client/packages/design-system/widgets/src/components/Checkbox/src/index.ts
- app/client/packages/design-system/widgets/src/components/Checkbox/src/types.ts
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/index.ts
Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/RadioGroup.tsx
- app/client/packages/design-system/widgets/src/components/RadioGroup/src/styles.module.css
Additional context used
Biome
app/client/packages/design-system/widgets/src/components/Checkbox/src/Checkbox.tsx
[error] 28-28: Avoid redundant
Booleancall (lint/complexity/noExtraBooleanCast)It is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall
Additional comments not posted (3)
app/client/packages/design-system/widgets/src/components/RadioGroup/src/types.ts (2)
4-10: LGTM! TheRadioGroupItemPropsinterface is well-defined and appropriately scoped for individual radio items.
12-34: Excellent documentation and structuring ofRadioGroupProps. The use ofkeyof typeof ORIENTATIONfor theorientationproperty enhances type safety and clarity.app/client/packages/design-system/widgets/src/components/Checkbox/src/Checkbox.tsx (1)
6-6: The update to the import statement forCheckboxPropsreflects the new organization of type definitions, aligning with best practices.
|
Deploy-Preview-URL: https://ce-34312.dp.appsmith.com |
Description
Fixes #27677
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/9567388261
Commit: 62d1153caa8bf03d827f88593c9dfaf3121091ee
Cypress dashboard.
Tags:
@tag.AnvilCommunication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
ErrorMessagecomponent for displaying error messages with specific styling.isDisabledprop forLabelandToggleGroupcomponents.RadioGroupto accept an items array for easier configuration.Bug Fixes
Labelcomponent to prevent issues whentextandcontextualHelpare both falsy.Refactor
Radiocomponent export from design system.CheckboxandRadioGroup.Style
RadioGroupandToggleGroupcomponents for better state handling (disabled, hovered, selected).Documentation
RadioGroupto reflect changes in component usage.Tests
RadioGroup.test.tsxto test new items array prop.