-
Notifications
You must be signed in to change notification settings - Fork 2
[SUR-488] [ForceUI] Add reverse position support for the bordered variant radio-button component #471
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: staging
Are you sure you want to change the base?
[SUR-488] [ForceUI] Add reverse position support for the bordered variant radio-button component #471
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,6 +157,81 @@ RadioWithBorderSmallSize.args = { | |
| size: 'sm', | ||
| }; | ||
|
|
||
| const RadioWithBorderReversePositionTemplate: StoryFn<RadioButtonGroupProps> = ( | ||
| args | ||
| ) => { | ||
| const [ value, setValue ] = useState( args.value || args.defaultValue ); | ||
|
|
||
| return ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What: In the new templates, you're repeating the setup code for RadioButton and Switch components. Consider creating a reusable function to encapsulate this logic. Why: Duplicating code can lead to maintenance issues since a change in one instance would require a change in all. It decreases readability and increases potential for bugs. How: You can create a helper function that returns the RadioButton.Group setup and just modify it as necessary for different scenarios. |
||
| <RadioButton.Group | ||
| value={ value } | ||
| columns={ args.columns ?? 3 } | ||
| onChange={ ( val ) => { | ||
| setValue( val as string ); | ||
| } } | ||
| { ...args } | ||
| > | ||
| { [ 1, 2, 3, 4, 5, 6 ].map( ( num ) => ( | ||
| <RadioButton.Button | ||
| key={ num } | ||
| value={ `option${ num }` } | ||
| label={ { | ||
| heading: `Option ${ num }`, | ||
| } } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What: The new RadioButton stories have added complexity with monitoring state management. Ensure that the initial state is handled carefully to avoid unintentional mutations or leaks in state management. Why: Improper handling of component state can lead to bugs that are difficult to trace and can affect usability and performance. How: Consider using functional updates to set state or utilizing React's useReducer for complex state logic to prevent inadvertent state bugs. |
||
| disabled={ args.disabled } | ||
| borderOn={ true } | ||
| reversePosition={ true } | ||
| /> | ||
| ) ) } | ||
| </RadioButton.Group> | ||
| ); | ||
| }; | ||
|
|
||
| export const RadioWithBorderReversePosition = | ||
| RadioWithBorderReversePositionTemplate.bind( {} ); | ||
| RadioWithBorderReversePosition.args = { | ||
| size: 'md', | ||
| }; | ||
|
|
||
| const SwitchWithBorderReversePositionTemplate: StoryFn< | ||
| RadioButtonGroupProps | ||
| > = ( args ) => { | ||
| const [ value, setValue ] = useState<string[]>( [] ); | ||
|
|
||
| return ( | ||
| <RadioButton.Group | ||
| value={ value } | ||
| columns={ args.columns ?? 2 } | ||
| multiSelection={ true } | ||
| onChange={ ( val ) => { | ||
| setValue( val as string[] ); | ||
| } } | ||
| { ...args } | ||
| > | ||
| { [ 1, 2, 3, 4 ].map( ( num ) => ( | ||
| <RadioButton.Button | ||
| key={ num } | ||
| value={ `option${ num }` } | ||
| label={ { | ||
| heading: `Option ${ num }`, | ||
| description: `Description ${ num }`, | ||
| } } | ||
| disabled={ args.disabled } | ||
| borderOn={ true } | ||
| reversePosition={ true } | ||
| useSwitch={ true } | ||
| /> | ||
| ) ) } | ||
| </RadioButton.Group> | ||
| ); | ||
| }; | ||
|
|
||
| export const SwitchWithBorderReversePosition = | ||
| SwitchWithBorderReversePositionTemplate.bind( {} ); | ||
| SwitchWithBorderReversePosition.args = { | ||
| size: 'md', | ||
| }; | ||
|
|
||
| const defaultRadioButtonGroupData = [ | ||
| { | ||
| id: '1', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,7 +101,7 @@ export interface RadioButtonProps extends RadioButtonCommonProps { | |
| inlineIcon?: boolean; | ||
| /** Hides the selection indicator */ | ||
| hideSelection?: boolean; | ||
| /** Reverses the position of icon and label */ | ||
| /** Places the selection control (radio/switch) on the left and the label on the right. Works with the bordered variant (`borderOn`) */ | ||
| reversePosition?: boolean; | ||
| /** Adds a border around the button */ | ||
| borderOn?: boolean; | ||
|
|
@@ -340,7 +340,6 @@ export const RadioButtonComponent = ( | |
| 'space-y-3': size === 'sm', | ||
| 'space-y-4': size === 'md', | ||
| }, | ||
| reversePosition && ( useSwitch ? 'ml-10' : 'ml-4' ), | ||
| inlineIcon && 'flex gap-2', | ||
| inlineIcon && ! label.description && 'items-center' | ||
| ) } | ||
|
|
@@ -358,7 +357,7 @@ export const RadioButtonComponent = ( | |
| > | ||
| <p | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What: Using Why: Maintaining a balance between CSS standards and performance is crucial. Overly broad or non-standard CSS can lead to unexpected behavior across different browsers, which can also potentially influence performance. How: Consider using a more standard CSS approach for overflowing text, such as setting |
||
| className={ cn( | ||
| 'text-text-primary font-medium m-0', | ||
| 'text-text-primary font-medium m-0 [overflow-wrap:anywhere]', | ||
| textSizeClassNames[ | ||
| size as keyof typeof textSizeClassNames | ||
| ], | ||
|
|
@@ -368,7 +367,7 @@ export const RadioButtonComponent = ( | |
| { label.heading } | ||
| </p> | ||
| { label.description && ( | ||
| <p className="text-text-tertiary text-sm font-normal leading-5 m-0"> | ||
| <p className="text-text-tertiary text-sm font-normal leading-5 m-0 [overflow-wrap:anywhere]"> | ||
| { label.description } | ||
| </p> | ||
| ) } | ||
|
|
@@ -405,6 +404,13 @@ export const RadioButtonComponent = ( | |
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What: The conditional assignment of Why: Improving conditional logic for class assignment can enhance code legibility and decrease potential for errors when toggling classes, especially as components evolve. How: Consider using the ternary operator directly in the class list like this: |
||
| }; | ||
|
|
||
| // Reserve space on the control side. Default: control on the right (pr-12). | ||
| // Reversed: control on the left; the switch needs more room than the radio. | ||
| let controlSpacingClass = 'pr-12'; | ||
| if ( reversePosition ) { | ||
| controlSpacingClass = useSwitch ? 'pl-16' : 'pl-12'; | ||
| } | ||
|
|
||
| const paddingClasses = { | ||
| 'pl-3.5 pr-2.5 py-2.5': size === 'sm' && ! ( icon && useSwitch ), | ||
| 'p-3': size === 'sm' && ( ( icon && useSwitch ) || ( icon && badgeItem ) ), | ||
|
|
@@ -425,7 +431,7 @@ export const RadioButtonComponent = ( | |
| checkedValue && | ||
| 'outline-border-interactive', | ||
| paddingClasses, | ||
| 'pr-12', | ||
| controlSpacingClass, | ||
| isDisabled && 'cursor-not-allowed opacity-40', | ||
| buttonWrapperClasses | ||
| ) } | ||
|
|
@@ -457,10 +463,10 @@ export const RadioButtonComponent = ( | |
| ) } | ||
| <label | ||
| className={ cn( | ||
| 'absolute mr-0.5 right-3 flex items-center cursor-pointer rounded-full gap-2', | ||
| reversePosition && 'left-0', | ||
| 'absolute flex items-center cursor-pointer rounded-full gap-2', | ||
| reversePosition ? 'left-3 ml-0.5' : 'right-3 mr-0.5', | ||
| isDisabled && 'cursor-not-allowed', | ||
| inlineIcon && 'mr-3' | ||
| inlineIcon && ( reversePosition ? 'ml-3' : 'mr-3' ) | ||
| ) } | ||
| onClick={ handleLabelClick } | ||
| > | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -231,6 +231,7 @@ const defaultTheme = { | |
| }, | ||
| borderWidth: { | ||
| 0.5: '0.5px', | ||
| 1.5: '1.5px', | ||
| }, | ||
| }, | ||
| }, | ||
|
|
||
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.
What: Consider adding comments to describe the purpose of new story components such as RadioWithBorderReversePosition and SwitchWithBorderReversePosition. It's not immediately clear what these stories are demonstrating or why they are useful.
Why: Adding comments will help other developers (or future you) understand the intended use and functionality of these stories, improving maintainability and clarity.
How: For instance, add comments like
// Story demonstrating the reverse position for bordered RadioButtonabove each story definition.