-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fix: Make the entire advanced mode toggle container clickable #7761
fix: Make the entire advanced mode toggle container clickable #7761
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.
PR Summary
This pull request modifies the AdvancedSettingsToggle component to improve usability by making the entire container clickable for toggling advanced mode.
- Added
cursor: pointer
andflex-grow: 1
toStyledText
inAdvancedSettingsToggle.tsx
- Introduced
toogleAdvancedMode
function to handle click events - Implemented
onClick
handler forStyledText
to toggle advanced mode - Enhanced user experience by allowing clicks anywhere in the Advanced Mode container
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-front/src/modules/ui/navigation/link/components/AdvancedSettingsToggle.tsx
Outdated
Show resolved
Hide resolved
const toggleAdvancedMode = () => { | ||
setIsAdvancedModeEnabled((current) => !current); | ||
}; | ||
|
||
return ( | ||
<StyledContainer> | ||
<StyledIconContainer> | ||
<StyledIconTool size={12} color={MAIN_COLORS.yellow} /> | ||
</StyledIconContainer> | ||
<StyledToggleContainer> |
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 we should make the Container clickable
@Devessier what's your recommendation here ? use a button?
I feel the "Advanced" is actually a label for the toggle
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.
Thanks for mentioning me, @charlesBochet.
Indeed, I think we should make the text "Advanced:" a true HTML <label>
element. I don't believe the whole container should be clickable, as clicking on a blank space would update the toggle, which might seem strange.
The issue is that the <Toggle />
component is only made of <div>
and not a proper <input>
element, so using a <label>
would do nothing. I had this component on my list of accessibility things to discuss.
What do you think we should do? I'm happy to discuss short-term/mid-term solutions.
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.
@Vardhaman619 could you update our Toggle component to include a
Take inspiration from chakra ui regarding the html structure
It looks to me that they have an invisible input
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.
Please note that the styles applied to hide the input
are to make it visible only for screen readers (the tool used by people with disabilities to visit websites).
These styles are now normalized and everybody uses the same to hide an element for people not using a screen reader:
// In Tailwind:
.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border-width: 0;
}
Some resources :
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.
We can introduce a component in this case, let's put it in an accessibility folder in twenty-ui
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.
Chakra UI has two components : VisibilityHidden and VisibilityHiddenInput. (https://v2.chakra-ui.com/docs/components/visually-hidden/usage#visually-hidden-input)
Their VisibilityHidden component renders a <span>
as it's usually used to provide little indications to the users, like a hidden label explaining what shows an icon.
I'm working on it. |
…el element for advanced settings
Thank you a lot for your contribution, @Vardhaman619. It was a critical bug fix, and I handled the update. |
Awarding Vardhaman619: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/Vardhaman619 |
In this PR:
<input type="checkbox" />
element in the<Toggle />
componentaccessibility
module in thetwenty-ui
packageVISIBILITY_HIDDEN
CSS object to hide visually any element<VisibilityHidden />
component from thetwenty-ui
package to add visually hidden textual information easily<VisibilityHiddenInput />
component to create custom form control components easily<label>
element for the "Advanced:" text; it will naturally toggle the advanced settingsFixes #7756