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

Jump to bottom

Fix: danger font color values & acc. to design specs #5344

Merged
merged 5 commits into from
May 20, 2024
Merged

Fix: danger font color values & acc. to design specs #5344

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
024f210
fix: fontLight & fontDark 'danger' color as per design spec
its-id May 9, 2024
cf55985
Replace theme.font.color.danger with theme.color.red
its-id May 9, 2024
9fe2eca
fix: hover color of StyledConfirmationButton in ConfirmationModal
its-id May 9, 2024
6eb9ae8
Revert
FelixMalfait May 20, 2024
feed1a8
Change color to match border
FelixMalfait May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...es/twenty-front/src/modules/activities/calendar/components/CalendarCurrentEventCursor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ type CalendarCurrentEventCursorProps = {

const StyledCurrentEventCursor = styled(motion.div)`
align-items: center;
background-color: ${({ theme }) => theme.font.color.danger};
background-color: ${({ theme }) => theme.color.red};
Copy link
Member

Choose a reason for hiding this comment

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

this should not be necessary if we keep font.color.danger = color.red, see comment below

display: inline-flex;
height: 1.5px;
left: 0;
Expand All @@ -32,7 +32,7 @@ const StyledCurrentEventCursor = styled(motion.div)`
transform: translateY(-50%);

&::before {
background-color: ${({ theme }) => theme.font.color.danger};
background-color: ${({ theme }) => theme.color.red};
Copy link
Member

Choose a reason for hiding this comment

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

this should not be necessary if we keep font.color.danger = color.red, see comment below

border-radius: 1px;
content: '';
display: block;
Expand Down
2 changes: 1 addition & 1 deletion packages/twenty-front/src/modules/activities/tasks/components/TaskRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const StyledDueDate = styled.div<{
}>`
align-items: center;
color: ${({ theme, isPast }) =>
isPast ? theme.font.color.danger : theme.font.color.secondary};
isPast ? theme.color.red : theme.font.color.secondary};
Copy link
Member

Choose a reason for hiding this comment

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

this should not be necessary if we keep font.color.danger = color.red, see comment below

display: flex;
gap: ${({ theme }) => theme.spacing(1)};
padding-left: ${({ theme }) => theme.spacing(2)};
Expand Down
2 changes: 1 addition & 1 deletion ...nty-front/src/modules/settings/developers/components/SettingsApiKeysFieldItemTableRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const SettingsApiKeysFieldItemTableRow = ({
<TableCell
color={
fieldItem.expiration === 'Expired'
? theme.font.color.danger
Copy link
Member

Choose a reason for hiding this comment

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

this should not be necessary if we keep font.color.danger = color.red, see comment below

? theme.color.red
: theme.font.color.tertiary
}
>
Expand Down
2 changes: 1 addition & 1 deletion packages/twenty-front/src/modules/ui/input/button/components/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ const StyledButton = styled.button<
box-shadow: ${!disabled && focus
? `0 0 0 3px ${theme.color.red10}`
: 'none'};
color: ${!disabled ? theme.font.color.danger : theme.color.red20};
color: ${!disabled ? theme.color.red : theme.font.color.danger};
Copy link
Member

Choose a reason for hiding this comment

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

@Bonapara could you provide the value of the red disabled button color for dark mode. I cannot find it in the Figma :(

For the light one, I see that you are actually not changing the font but changing the overall opacity which could be an issue depending what's behind the button. is this intended?

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Per discussion with Bonapara, we should update Button and IconButton to have same font as non disabled but opacity to 0.24%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. What I did was follow the same color design being used by the border's light and dark variant in disabled mode. since, button's border is working fine as expected.

I was trying to use the existing values without introducing any new color values which basically work great and is tested as well on multiple pages with dark and light mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Bonapara could you provide the value of the red disabled button color for dark mode. I cannot find it in the Figma :(

For the light one, I see that you are actually not changing the font but changing the overall opacity which could be an issue depending what's behind the button. is this intended?

image

I don't think there is an opacity added to the color. the colors red20 and red70 do not have any opacity as I can see.

&:hover {
background: ${!disabled
? theme.background.danger
Expand Down
2 changes: 1 addition & 1 deletion packages/twenty-front/src/modules/ui/input/button/components/IconButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ const StyledButton = styled.button<
box-shadow: ${!disabled && focus
? `0 0 0 3px ${theme.color.red10}`
: 'none'};
color: ${!disabled ? theme.font.color.danger : theme.color.red20};
color: ${!disabled ? theme.color.red : theme.font.color.danger};
Copy link
Member

Choose a reason for hiding this comment

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

same as above here

&:hover {
background: ${!disabled
? theme.background.danger
Expand Down
2 changes: 1 addition & 1 deletion packages/twenty-front/src/modules/ui/input/components/ImageInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const StyledText = styled.span`
`;

const StyledErrorText = styled.span`
color: ${({ theme }) => theme.font.color.danger};
color: ${({ theme }) => theme.color.red};
Copy link
Member

Choose a reason for hiding this comment

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

this should not be necessary if we keep font.color.danger = color.red, see comment below

font-size: ${({ theme }) => theme.font.size.xs};
margin-top: ${({ theme }) => theme.spacing(1)};
`;
Expand Down
2 changes: 1 addition & 1 deletion packages/twenty-front/src/modules/ui/layout/modal/components/ConfirmationModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export const StyledConfirmationButton = styled(StyledCenteredButton)`
font-size: ${({ theme }) => theme.font.size.md};
line-height: ${({ theme }) => theme.text.lineHeight.lg};
:hover {
background-color: ${({ theme }) => theme.color.red10};
background-color: ${({ theme }) => theme.background.danger};
}
`;

Expand Down
2 changes: 1 addition & 1 deletion ...nty-front/src/modules/ui/navigation/menu-item/internals/components/StyledMenuItemBase.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const StyledMenuItemBase = styled.li<MenuItemBaseProps>`
switch (accent) {
case 'danger': {
return css`
color: ${theme.font.color.danger};
color: ${theme.color.red};
Copy link
Member

Choose a reason for hiding this comment

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

this should not be necessary if we keep font.color.danger = color.red, see comment below

&:hover {
background: ${theme.background.transparent.danger};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/twenty-ui/src/theme/constants/FontDark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export const FONT_DARK = {
light: GRAY_SCALE.gray50,
extraLight: GRAY_SCALE.gray55,
inverted: GRAY_SCALE.gray100,
danger: COLOR.red,
danger: COLOR.red70,
Copy link
Member

Choose a reason for hiding this comment

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

@Bonapara I feel this was right before @its-id change.
the danger color font should be red #f83e3e according to Figma

Could you confirm?

Copy link
Contributor Author

@its-id its-id May 15, 2024 •
edited

Choose a reason for hiding this comment

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

So, to understand the fix: a brief about the issue itself. Earlier the FontLight and FontDark both were hardcoded to take only single color which turned out to be an issue in disabled mode. (ref to issue screenshot here)

The fix revolved around the usage of FontDark and FontLight to work same as BorderLight and BorderDark in disabled mode. So, I made FontDark and FontLight danger color to use same value same as being used by BorderLight (#fed8d8) and BorderDark (#441f1f).

And as for the other files which were previously using font.color.danger value which was basically COLOR.red (#f83e3e), I switched to theme.color.red making sure their color value doesn't change.

This now becomes much more cleaner and easier to use. So, acc. to current changes, fontDark and fontLight danger color is being used only for disabled mode.

},
...FONT_COMMON,
};
2 changes: 1 addition & 1 deletion packages/twenty-ui/src/theme/constants/FontLight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export const FONT_LIGHT = {
light: GRAY_SCALE.gray35,
extraLight: GRAY_SCALE.gray30,
inverted: GRAY_SCALE.gray0,
danger: COLOR.red,
danger: COLOR.red20,
Copy link
Member

Choose a reason for hiding this comment

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

same here

},
...FONT_COMMON,
};