Skip to content
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

Merged

Conversation

Vardhaman619
Copy link
Contributor

@Vardhaman619 Vardhaman619 commented Oct 16, 2024

In this PR:

  • Use a real <input type="checkbox" /> element in the <Toggle /> component
  • Create an accessibility module in the twenty-ui package
  • Export the VISIBILITY_HIDDEN CSS object to hide visually any element
  • Export a <VisibilityHidden /> component from the twenty-ui package to add visually hidden textual information easily
  • Export a <VisibilityHiddenInput /> component to create custom form control components easily
  • Use a <label> element for the "Advanced:" text; it will naturally toggle the advanced settings

Fixes #7756

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 and flex-grow: 1 to StyledText in AdvancedSettingsToggle.tsx
  • Introduced toogleAdvancedMode function to handle click events
  • Implemented onClick handler for StyledText 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

const toggleAdvancedMode = () => {
setIsAdvancedModeEnabled((current) => !current);
};

return (
<StyledContainer>
<StyledIconContainer>
<StyledIconTool size={12} color={MAIN_COLORS.yellow} />
</StyledIconContainer>
<StyledToggleContainer>
Copy link
Member

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

Copy link
Contributor

@Devessier Devessier Oct 18, 2024

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.

See https://github.com/twentyhq/twenty/blob/05e8f8a0b1fd4c0b1e53c289baba3d073565b61a/packages/twenty-front/src/modules/ui/input/components/Toggle.tsx

What do you think we should do? I'm happy to discuss short-term/mid-term solutions.

Copy link
Member

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
image

It looks to me that they have an invisible input

Copy link
Contributor

@Devessier Devessier Oct 18, 2024

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 :

Copy link
Member

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

Copy link
Contributor

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.

@Devessier
Copy link
Contributor

I'm working on it.

@Devessier
Copy link
Contributor

Thank you a lot for your contribution, @Vardhaman619. It was a critical bug fix, and I handled the update.

@Devessier Devessier removed their assignment Oct 21, 2024
@charlesBochet charlesBochet merged commit 1466d44 into twentyhq:main Oct 21, 2024
16 checks passed
Copy link

oss-gg bot commented Oct 21, 2024

Awarding Vardhaman619: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/Vardhaman619

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make the whole advanced mode container clickable
3 participants