-
Notifications
You must be signed in to change notification settings - Fork 1
IBX-10895: Fixed RadioButton issues #85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes RadioButton component issues by refactoring prop handling, adding state synchronization, and improving styling for disabled and error states.
- Refactored
requiredprop handling by moving it fromBaseChoiceInputtoBaseInputVisibleProps - Added
useEffecthooks in HOCs to synchronize internal state with external prop changes - Enhanced disabled and error state styling for choice input fields
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storybook/decorators/FormDecorator.tsx | New reusable form decorator to replace inline decorators in stories |
| packages/components/src/partials/BaseInput/BaseInput.types.ts | Made required prop optional in BaseInputVisibleProps |
| packages/components/src/partials/BaseChoiceInputField/BaseChoiceInputField.types.ts | Added disabled and error props to support state-based styling |
| packages/components/src/partials/BaseChoiceInputField/BaseChoiceInputField.tsx | Applied disabled and error CSS modifiers based on new props |
| packages/components/src/partials/BaseChoiceInput/BaseChoiceInput.types.ts | Removed required prop from BaseChoiceInput interface |
| packages/components/src/partials/BaseChoiceInput/BaseChoiceInput.tsx | Removed required prop usage from component implementation |
| packages/components/src/hoc/withStateValue.tsx | Added useEffect to sync component state with external value prop changes |
| packages/components/src/hoc/withStateChecked.tsx | Added useEffect to sync component state with external checked prop changes |
| packages/components/src/components/RadioButton/RadioButtonsListField/RadioButtonsListField.stories.tsx | Applied new FormDecorator to story |
| packages/components/src/components/RadioButton/RadioButtonInput/RadioButtonInput.stories.tsx | Replaced inline form decorator with reusable FormDecorator |
| packages/components/src/components/RadioButton/RadioButtonField/RadioButtonField.tsx | Passed through disabled and error props to BaseChoiceInputField |
| packages/components/src/components/RadioButton/RadioButtonField/RadioButtonField.stories.tsx | Applied new FormDecorator to story |
| packages/assets/src/scss/mixins/_inputs.scss | Prevented hover styles from applying when input is disabled |
| packages/assets/src/scss/_choice-input-field.scss | Added comprehensive disabled, error, and hover state styling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| useEffect(() => { | ||
| setComponentValue(value); | ||
| }, [value]); |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect hook should also include setComponentValue in its dependency array to follow React Hook exhaustive-deps rules. While setComponentValue is stable due to being from useState, explicitly including it ensures compliance with linting rules and makes dependencies clear.
| }, [value]); | |
| }, [value, setComponentValue]); |
|
|
||
| useEffect(() => { | ||
| setComponentChecked(checked); | ||
| }, [checked]); |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect hook should also include setComponentChecked in its dependency array to follow React Hook exhaustive-deps rules. While setComponentChecked is stable due to being from useState, explicitly including it ensures compliance with linting rules and makes dependencies clear.
| }, [checked]); | |
| }, [checked, setComponentChecked]); |
23cf9da to
43d3db5
Compare
Related PRs:
Description:
For QA:
Documentation: