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.

Sign up for GitHub

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
merged 6 commits into from
Oct 21, 2024
Merged

fix: Make the entire advanced mode toggle container clickable #7761

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fbaace5
fix: Make the entire advanced mode toggle container clickable
Vardhaman619 Oct 16, 2024
d9f2503
fix: spell mistake
Vardhaman619 Oct 16, 2024
152b3dc
Revert "fix: spell mistake"
Devessier Oct 21, 2024
580bc20
Revert "fix: Make the entire advanced mode toggle container clickable"
Devessier Oct 21, 2024
9961c9d
feat: improve the accessibility of the toggle component and use a lab…
Devessier Oct 21, 2024
debca26
refactor: remove manual forwardRef
Devessier Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/twenty-front/src/modules/ui/navigation/link/components/AdvancedSettingsToggle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const StyledText = styled.span`
font-size: ${({ theme }) => theme.font.size.sm};
font-weight: ${({ theme }) => theme.font.weight.medium};
padding: ${({ theme }) => theme.spacing(1)};
cursor: pointer;
flex-grow: 1;
`;

const StyledIconContainer = styled.div`
Expand Down Expand Up @@ -46,13 +48,17 @@ export const AdvancedSettingsToggle = () => {
setIsAdvancedModeEnabled(newValue);
};

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 •
edited
Loading

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 •
edited
Loading

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.

<StyledText>Advanced:</StyledText>
<StyledText onClick={toggleAdvancedMode}>Advanced:</StyledText>
<Toggle
onChange={onChange}
color={MAIN_COLORS.yellow}
Expand Down
Loading