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 noninteractive toggle #8383

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Fix noninteractive toggle #8383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
669945a
fix: make the whole setting card interactive
Devessier Nov 7, 2024
9a797fa
fix: ensure the toggle never shrinks
Devessier Nov 7, 2024
5cf4d37
lint: delete unused component
Devessier Nov 7, 2024
512aa1f
refactor: simplify the components
Devessier Nov 7, 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
19 changes: 6 additions & 13 deletions ...front/src/modules/settings/accounts/components/SettingsAccountsCalendarChannelDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { SettingsAccountsEventVisibilitySettingsCard } from '@/settings/accounts
import { SettingsOptionCardContent } from '@/settings/components/SettingsOptionCardContent';
import styled from '@emotion/styled';
import { Section } from '@react-email/components';
import { Card, H2Title, Toggle } from 'twenty-ui';
import { Card, H2Title } from 'twenty-ui';
import { CalendarChannelVisibility } from '~/generated-metadata/graphql';

const StyledDetailsContainer = styled.div`
Expand All @@ -21,10 +21,6 @@ type SettingsAccountsCalendarChannelDetailsProps = {
>;
};

const StyledToggle = styled(Toggle)`
margin-left: auto;
`;

export const SettingsAccountsCalendarChannelDetails = ({
calendarChannel,
}: SettingsAccountsCalendarChannelDetailsProps) => {
Expand Down Expand Up @@ -71,16 +67,13 @@ export const SettingsAccountsCalendarChannelDetails = ({
<SettingsOptionCardContent
title="Auto-creation"
description="Automatically create contacts for people."
onClick={() =>
checked={calendarChannel.isContactAutoCreationEnabled}
onChange={() => {
handleContactAutoCreationToggle(
!calendarChannel.isContactAutoCreationEnabled,
)
}
>
<StyledToggle
value={calendarChannel.isContactAutoCreationEnabled}
/>
</SettingsOptionCardContent>
);
}}
Comment on lines +71 to +75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Unnecessary arrow function wrapper. Pass handleContactAutoCreationToggle directly as the onChange prop.

/>
</Card>
</Section>
</StyledDetailsContainer>
Expand Down
24 changes: 9 additions & 15 deletions ...-front/src/modules/settings/accounts/components/SettingsAccountsMessageChannelDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import styled from '@emotion/styled';
import { Card, H2Title, Section, Toggle } from 'twenty-ui';
import { Card, H2Title, Section } from 'twenty-ui';

import {
MessageChannel,
Expand Down Expand Up @@ -30,10 +30,6 @@ const StyledDetailsContainer = styled.div`
gap: ${({ theme }) => theme.spacing(6)};
`;

const StyledToggle = styled(Toggle)`
margin-left: auto;
`;

export const SettingsAccountsMessageChannelDetails = ({
messageChannel,
}: SettingsAccountsMessageChannelDetailsProps) => {
Expand Down Expand Up @@ -107,25 +103,23 @@ export const SettingsAccountsMessageChannelDetails = ({
title="Exclude non-professional emails"
description="Don’t create contacts from/to Gmail, Outlook emails"
divider
onClick={() =>
checked={messageChannel.excludeNonProfessionalEmails}
onChange={() => {
handleIsNonProfessionalEmailExcludedToggle(
!messageChannel.excludeNonProfessionalEmails,
)
}
>
<StyledToggle value={messageChannel.excludeNonProfessionalEmails} />
</SettingsOptionCardContent>
);
}}
/>
<SettingsOptionCardContent
title="Exclude group emails"
description="Don’t sync emails from team@ support@ noreply@..."
onClick={() =>
checked={messageChannel.excludeGroupEmails}
onChange={() =>
handleIsGroupEmailExcludedToggle(
!messageChannel.excludeGroupEmails,
)
}
>
<StyledToggle value={messageChannel.excludeGroupEmails} />
</SettingsOptionCardContent>
/>
</Card>
</Section>
</StyledDetailsContainer>
Expand Down
43 changes: 32 additions & 11 deletions packages/twenty-front/src/modules/settings/components/SettingsOptionCardContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
import styled from '@emotion/styled';
import { useTheme } from '@emotion/react';
import styled from '@emotion/styled';

import { IconComponent, CardContent } from 'twenty-ui';
import { ReactNode } from 'react';
import { useId } from 'react';
import { CardContent, IconComponent, Toggle } from 'twenty-ui';

type SettingsOptionCardContentProps = {
Icon?: IconComponent;
title: string;
title: React.ReactNode;
description: string;
onClick: () => void;
children: ReactNode;
divider?: boolean;
checked: boolean;
onChange: (checked: boolean) => void;
};

const StyledCardContent = styled(CardContent)`
align-items: center;
display: flex;
gap: ${({ theme }) => theme.spacing(4)};
cursor: pointer;
position: relative;

Comment on lines 20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: cursor: pointer without click handler may be misleading to users

&:hover {
background: ${({ theme }) => theme.background.transparent.lighter};
Expand Down Expand Up @@ -47,28 +48,48 @@ const StyledIcon = styled.div`
min-width: ${({ theme }) => theme.icon.size.md};
`;

const StyledToggle = styled(Toggle)`
margin-left: auto;
`;

const StyledCover = styled.span`
cursor: pointer;
inset: 0;
position: absolute;
`;

export const SettingsOptionCardContent = ({
Icon,
title,
description,
onClick,
children,
divider,
checked,
onChange,
}: SettingsOptionCardContentProps) => {
const theme = useTheme();

const toggleId = useId();

return (
<StyledCardContent onClick={onClick} divider={divider}>
<StyledCardContent divider={divider}>
{Icon && (
<StyledIcon>
<Icon size={theme.icon.size.md} stroke={theme.icon.stroke.md} />
</StyledIcon>
)}

<div>
<StyledTitle>{title}</StyledTitle>
<StyledTitle>
<label htmlFor={toggleId}>
{title}

<StyledCover />
</label>
</StyledTitle>
<StyledDescription>{description}</StyledDescription>
</div>
{children}

<StyledToggle id={toggleId} value={checked} onChange={onChange} />
</StyledCardContent>
);
};
21 changes: 10 additions & 11 deletions ...ges/twenty-front/src/modules/settings/security/components/SettingsSecurityOptionsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@ import { currentWorkspaceState } from '@/auth/states/currentWorkspaceState';
import { SettingsOptionCardContent } from '@/settings/components/SettingsOptionCardContent';
import { SnackBarVariant } from '@/ui/feedback/snack-bar-manager/components/SnackBar';
import { useSnackBar } from '@/ui/feedback/snack-bar-manager/hooks/useSnackBar';
import styled from '@emotion/styled';
import { useRecoilState } from 'recoil';
import { Card, IconLink, Toggle } from 'twenty-ui';
import { Card, IconLink, isDefined } from 'twenty-ui';
import { useUpdateWorkspaceMutation } from '~/generated/graphql';

const StyledToggle = styled(Toggle)`
margin-left: auto;
`;

export const SettingsSecurityOptionsList = () => {
const { enqueueSnackBar } = useSnackBar();

const [currentWorkspace, setCurrentWorkspace] = useRecoilState(
currentWorkspaceState,
);
if (!isDefined(currentWorkspace)) {
throw new Error(
'The current workspace must be defined to edit its security options.',
);
}

const [updateWorkspace] = useUpdateWorkspaceMutation();

Expand Down Expand Up @@ -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}
Copy link
Contributor

@AMoreaux AMoreaux Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

onChange={() =>
handleChange(!currentWorkspace.isPublicInviteLinkEnabled)
}
>
<StyledToggle value={currentWorkspace?.isPublicInviteLinkEnabled} />
</SettingsOptionCardContent>
/>
</Card>
);
};
1 change: 1 addition & 0 deletions packages/twenty-ui/src/input/components/Toggle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ type ContainerProps = {
};

const StyledContainer = styled.label<ContainerProps>`
flex-shrink: 0;
align-items: center;
background-color: ${({ theme, isOn, color }) =>
isOn ? (color ?? theme.color.blue) : theme.background.transparent.medium};
Expand Down
Loading