refactor(forms): simplify form handling and button behavior #10441
Conversation
Removed redundant handleSave and handleSubmit props in domain settings. Integrated form submission logic directly into form components, ensuring consistent behavior and reducing complexity. Updated button components to explicitly support the "type" attribute for improved accessibility and functionality.
Introduced a new type prop to the LightButton component, allowing better control over button behavior (e.g., submit, reset). This enhances flexibility while maintaining compatibility with existing usage.
| @@ -15,6 +15,7 @@ export const SaveButton = ({ onSave, disabled }: SaveButtonProps) => { | |||
| accent="blue" | |||
| disabled={disabled} | |||
| onClick={onSave} | |||
| type={'submit'} | |||
| @@ -15,6 +15,7 @@ export const SaveButton = ({ onSave, disabled }: SaveButtonProps) => { | |||
| accent="blue" | |||
| disabled={disabled} | |||
| onClick={onSave} | |||
| type={'submit'} | |||
There was a problem hiding this comment.
Curly braces are not required as it's a static string
| type={'submit'} | |
| type="submit" |
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter') { | ||
| handleSubmit(handleSave); | ||
| } | ||
| }} |
There was a problem hiding this comment.
I think deleting the keydown event listener works only because we have a single input in this form. That's great as long as we don't need to add another input to the form. Great 👌
There was a problem hiding this comment.
It's more because there is 1 button with the type submit and the other input has the type "button".
Since I wrap the form in <form>...</form> the form submission is link the enter key.
There was a problem hiding this comment.
I think you don't need to have a button[type=submit] for the Enter key to submit the form by default. To me, the key is to have a single input inside the form. If you have two of them, it won't work. This is understandable as you might not have completed the form, so you might not want to submit the whole form automatically.
It's always interesting to go back to raw HTML 👌
CleanShot.2025-02-24.at.15.08.43.mp4
There was a problem hiding this comment.
When we set the right attributes we control who submits the form and leverage the browser features:
There was a problem hiding this comment.
And since we use the component SaveAndCancel we have two buttons and the submission should happen with the key enter and the click on save.
I try to improve the navigation with tabs In settings forms
Set correct "button" types for non-submit actions to prevent unintended form submissions. Refactored form configuration to enhance clarity and ensured consistent handling of form state and user feedback messages across components.
…nto chore/improve-form-submission
Reorganized component structures for better semantics and ARIA support, including improved keyboard navigation and role attributes. Adjusted button and form styling for consistent theming and added focus state management for buttons. Enhanced user experience with clipboard copy notifications and accessibility improvements in radio card components.
There was a problem hiding this comment.
It's nice to improve the accessibility of the application. I think we should discuss a global strategy for the radio element. Let's talk with @charlesBochet!
| }: ButtonProps) => { | ||
| const theme = useTheme(); | ||
|
|
||
| const isMobile = useIsMobile(); | ||
|
|
||
| const [isFocused, setIsFocused] = React.useState(propFocus); |
There was a problem hiding this comment.
I don't see the benefit of this state. Could you please explain it to me? Ideally, the focus should not be set through a prop, as the prop will never be the source of truth. Nonetheless, the isFocused state doesn't seem to do anything here. Did I miss something?
| const onKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => { | ||
| if (e.key === 'Enter' || e.key === ' ' || e.code === 'Space') { | ||
| e.preventDefault(); | ||
| handleSelect(value); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Ideally we should use hotkey scopes for keyboard management.
An ideal implementation of a radio group would switch option when clicking on down and up arrows.
| @@ -136,6 +137,7 @@ export const SettingsSSOSAMLForm = () => { | |||
| Icon={IconUpload} | |||
| onClick={handleUploadFileClick} | |||
| title={t`Upload file`} | |||
| type="button" | |||
There was a problem hiding this comment.
looks to be the default (type='button') do we need to specify it everytime?
There was a problem hiding this comment.
The default is type="submit" 😅 The change makes sense if the button is used in a <form> HTML element and mustn't trigger the form submission. @AMoreaux, do you confirm the use case of these buttons?
Removed redundant handleSave and handleSubmit props in domain settings. Integrated form submission logic directly into form components, ensuring consistent behavior and reducing complexity. Updated button components to explicitly support the "type" attribute for improved accessibility and functionality.