-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: organize settings dialog with tabs #301
Conversation
|
||
type SettingsDialogProps = { | ||
open: boolean | ||
onOpenChange: (open: boolean) => void | ||
} | ||
|
||
const tabs = [ |
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.
I extracted the content of the tabs to an array, so we can loop through and select which one to focus on after an error happens, following the order they are rendered in the UI. This ensures that the user is taken to the very first error at the top of the form.
This loop happens in the onInvalid
method on L72.
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.
so we can loop through and select which one to focus on after an error happens
Looks like saving on blur/change can help us avoid the problem altogether: we won't need to show the warning icon and focus on a broken tab, and the user won't need to remember to save when the changes they made are on a different tab
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.
I think using onBlur
would be ideal, however this won't work for fields that are dependant on a checkbox (such as the proxy credentials) as the checkbox uses onChange
instead.
@going-confetti and I agreed to leave as is for now and improve in the future.
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.
Looks good!
I wonder if it makes sense for the Recorder
section to show greyed out the inputs for selecting a specific browser... it feels like the information that you can actually do that it's hidden away 🤔
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.
LGTM!
@cristianoventura and I talked about auto-saving changes and it looks like it'll be a separate improvement later on, because of how settings are designed (for example, changing the proxy mode changes how the form should be validated)
Description
This PR changes the Settings dialog by separating sections into tabs. This provides a cleaner user interface as we add more functionalities to the Studio and enable users with custom settings.
How to Test
Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Screen.Recording.2024-11-05.at.4.13.41.PM.mov
Related PR(s)/Issue(s)