Fix noninteractive toggle #8383
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR enhances toggle component accessibility and mobile responsiveness across settings pages by introducing a new SettingsOptionCardContentWithToggle component and improving existing toggle implementations.
- Added new
SettingsOptionCardContentWithTogglecomponent in/packages/twenty-front/src/modules/settings/components/SettingsOptionCardContentWithToggle.tsxwith label-based click handling - Modified
/packages/twenty-ui/src/input/components/Toggle.tsxto prevent shrinking on mobile withflex-shrink: 0 - Replaced direct click handlers with
onChangehandlers across settings components for better accessibility - Added
useIdhook for toggle accessibility in card content components - Standardized toggle behavior across calendar, message, and security settings pages
6 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
| onChange={() => { | ||
| handleContactAutoCreationToggle( | ||
| !calendarChannel.isContactAutoCreationEnabled, | ||
| ) | ||
| } | ||
| > | ||
| <StyledToggle | ||
| value={calendarChannel.isContactAutoCreationEnabled} | ||
| /> | ||
| </SettingsOptionCardContent> | ||
| ); | ||
| }} |
There was a problem hiding this comment.
style: Unnecessary arrow function wrapper. Pass handleContactAutoCreationToggle directly as the onChange prop.
| <SettingsOptionCardContentWithToggle | ||
| title="Auto-creation" | ||
| description="Automatically create contacts for people." | ||
| onClick={() => | ||
| checked={calendarChannel.isContactAutoCreationEnabled} | ||
| onChange={() => { | ||
| handleContactAutoCreationToggle( | ||
| !calendarChannel.isContactAutoCreationEnabled, | ||
| ) | ||
| } | ||
| > | ||
| <StyledToggle | ||
| value={calendarChannel.isContactAutoCreationEnabled} | ||
| /> | ||
| </SettingsOptionCardContent> | ||
| ); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
style: Missing aria-label for better screen reader support since this toggle affects important functionality.
| onChange={() => { | ||
| handleIsNonProfessionalEmailExcludedToggle( | ||
| !messageChannel.excludeNonProfessionalEmails, | ||
| ) | ||
| } | ||
| > | ||
| <StyledToggle value={messageChannel.excludeNonProfessionalEmails} /> | ||
| </SettingsOptionCardContent> | ||
| <SettingsOptionCardContent | ||
| ); | ||
| }} |
There was a problem hiding this comment.
style: Unnecessary arrow function wrapper adds complexity. Can directly pass the handler
| cursor: pointer; | ||
| position: relative; |
There was a problem hiding this comment.
logic: cursor: pointer without click handler may be misleading to users
| const StyledCover = styled.span` | ||
| cursor: pointer; | ||
| inset: 0; | ||
| position: absolute; | ||
| `; |
There was a problem hiding this comment.
style: StyledCover overlays the entire card area but may interfere with text selection. Consider using pointer-events: none on text elements if needed.
| <label htmlFor={toggleId}> | ||
| {title} | ||
|
|
||
| <StyledCover /> | ||
| </label> | ||
| } |
There was a problem hiding this comment.
logic: Label wrapping may cause accessibility issues since it contains a clickable cover. Consider using aria-labelledby instead.
| Icon={Icon} | ||
| divider={divider} | ||
| > | ||
| <StyledToggle id={toggleId} value={checked} onChange={onChange} /> |
There was a problem hiding this comment.
logic: Missing aria-label on toggle for screen readers when title is not directly associated
Devessier
force-pushed
the
fix-noninteractive-toggle
branch
from
30d969a to
512aa1f
Compare
| @@ -49,12 +49,11 @@ export const SettingsSecurityOptionsList = () => { | |||
| Icon={IconLink} | |||
| title="Invite by Link" | |||
| description="Allow the invitation of new users by sharing an invite link." | |||
| onClick={() => | |||
| handleChange(!currentWorkspace?.isPublicInviteLinkEnabled) | |||
| checked={currentWorkspace.isPublicInviteLinkEnabled} | |||
There was a problem hiding this comment.
@Devessier I'm encountering a problem with this implementation. Since we need to use inputs other than a checkbox, we should use a slot instead of a prop.
https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=42308-234697&t=YrBrHCCvLIyihv7X-4
There was a problem hiding this comment.
Great point. We chose to restrict minimize the API surface with @charlesBochet as all Cards were relying on a Toggle component. We can modify the API have one version for toggles and one for inputs, etc.
There was a problem hiding this comment.
WDYT of this implementation:
https://github.com/twentyhq/twenty/pull/8378/files#diff-778c43a88bc2aed0767352fe3e86f1d8a49204de88fd23a6bd891127e0a4a98cR183-R196
We keep the label implementation with the flexibility to use custom input in the card.
There was a problem hiding this comment.
Great point. We chose to restrict minimize the API surface with @charlesBochet as all Cards were relying on a Toggle component. We can modify the API have one version for toggles and one for inputs, etc.
I didn't saw the answer 😅
I'll rollback. Thx
A focus indicator is missing for the Toggle component. We'll have to add one.