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

feat(auth/logo): Add dynamic sizing and conditional twenty logo #8587

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+70 −44
Open

feat(auth/logo): Add dynamic sizing and conditional twenty logo #8587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c3024f7
feat(auth/logo): Add dynamic sizing and conditional twenty logo
AMoreaux Nov 19, 2024
c62ef0e
refactor(Logo): Update StyledMainLogo type definition
AMoreaux Nov 19, 2024
9cbbe51
refactor(Logo): simplify StyledMainLogo component usage
AMoreaux Nov 19, 2024
bbe4baf
refactor(Logo): support primary and secondary logos
AMoreaux Nov 20, 2024
68580bb
refactor: implement `Spacing` type for consistent units
AMoreaux Nov 20, 2024
68ec09a
Merge branch 'main' into feat/front-improve-component-logo
FelixMalfait Nov 22, 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
76 changes: 44 additions & 32 deletions packages/twenty-front/src/modules/auth/components/Logo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,67 +1,79 @@
import { useTheme } from '@emotion/react';
import styled from '@emotion/styled';

import { getImageAbsoluteURI } from '~/utils/image/getImageAbsoluteURI';
import { Spacing } from 'twenty-ui';

type LogoProps = {
workspaceLogo?: string | null;
primaryLogo?: string | null;
secondaryLogo?: string | null;
size?: Spacing | null;
};

const StyledContainer = styled.div`
height: 48px;
const StyledContainer = styled.div<{ size: number }>`
height: ${({ size }) => size}px;
margin-bottom: ${({ theme }) => theme.spacing(4)};
margin-top: ${({ theme }) => theme.spacing(4)};

position: relative;
width: 48px;
width: ${({ size }) => size}px;
`;

const StyledTwentyLogo = styled.img`
const StyledSecondaryLogo = styled.img<{ size: number }>`
border-radius: ${({ theme }) => theme.border.radius.xs};
height: 24px;
width: 24px;
height: calc(${({ size }) => size}px / 2);
width: calc(${({ size }) => size}px / 2);
`;

const StyledTwentyLogoContainer = styled.div`
const StyledSecondaryLogoContainer = styled.div<{ size: number }>`
align-items: center;
background-color: ${({ theme }) => theme.background.primary};
border-radius: ${({ theme }) => theme.border.radius.sm};
bottom: ${({ theme }) => `-${theme.spacing(3)}`};
bottom: calc(
(${({ theme }) => theme.spacing(3)} * -1) * ${({ size }) => size} / 48
);
display: flex;
height: 28px;
height: calc(
(${({ theme }) => theme.spacing(7)} * ${({ size }) => size}) / 48
);
justify-content: center;

position: absolute;
right: ${({ theme }) => `-${theme.spacing(3)}`};
width: 28px;
right: calc(
(${({ theme }) => theme.spacing(3)} * -1) * ${({ size }) => size} / 48
);
width: calc(${({ theme }) => theme.spacing(7)} * ${({ size }) => size} / 48);
`;

type StyledMainLogoProps = {
logo?: string | null;
};

const StyledMainLogo = styled.div<StyledMainLogoProps>`
background: url(${(props) => props.logo});
const StyledPrimaryLogo = styled.div<{ src: string }>`
background: url(${(props) => props.src});
background-size: cover;
height: 100%;

width: 100%;
`;

export const Logo = ({ workspaceLogo }: LogoProps) => {
if (!workspaceLogo) {
return (
<StyledContainer>
<StyledMainLogo logo="/icons/android/android-launchericon-192-192.png" />
</StyledContainer>
);
}
export const Logo = (props: LogoProps) => {
const theme = useTheme();

const size = props.size ?? theme.spacing(12);

const defaultPrimaryLogoUrl = `${window.location.origin}/icons/android/android-launchericon-192-192.png`;

const primaryLogoUrl = getImageAbsoluteURI(
props.primaryLogo ?? defaultPrimaryLogoUrl,
);
const secondaryLogoUrl = getImageAbsoluteURI(props.secondaryLogo);

const sizeInNumber = parseInt(size, 10);

return (
<StyledContainer>
<StyledMainLogo logo={getImageAbsoluteURI(workspaceLogo)} />
<StyledTwentyLogoContainer>
<StyledTwentyLogo src="/icons/android/android-launchericon-192-192.png" />
</StyledTwentyLogoContainer>
<StyledContainer size={sizeInNumber}>
<StyledPrimaryLogo src={primaryLogoUrl} />
{secondaryLogoUrl && (
<StyledSecondaryLogoContainer size={sizeInNumber}>
<StyledSecondaryLogo size={sizeInNumber} src={secondaryLogoUrl} />
</StyledSecondaryLogoContainer>
)}
</StyledContainer>
);
};
2 changes: 1 addition & 1 deletion packages/twenty-front/src/pages/auth/Invite.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export const Invite = () => {
return (
<>
<AnimatedEaseIn>
<Logo workspaceLogo={workspaceFromInviteHash?.logo} />
<Logo secondaryLogo={workspaceFromInviteHash?.logo} />
</AnimatedEaseIn>
<Title animate>{title}</Title>
{isDefined(currentWorkspace) ? (
Expand Down
21 changes: 16 additions & 5 deletions packages/twenty-front/src/utils/image/getImageAbsoluteURI.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,26 @@
import { REACT_APP_SERVER_BASE_URL } from '~/config';

export const getImageAbsoluteURI = (imageUrl?: string | null) => {
type ImageAbsoluteURI<T extends string | null | undefined> = T extends string
Copy link
Member

Choose a reason for hiding this comment

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

this feels odd, not sure what issue it was trying to solve but we can probably do better no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It enables the inference of the type. When you use the function with a specified parameter, you can be confident that the return type will be determined. As a result, you won't need to add a condition to verify the return type. For example:

Before:

const myPath = '/img.png'
const myImage = getImageAbsoluteURI(myPath);
// myImage type is string | undefined si I will need to check the type

Now:

const myPath = '/img.png'
const myImage = getImageAbsoluteURI(myPath);
// myImage type is string.  
const myPath: string | null;
const myImage = getImageAbsoluteURI(myPath);
// myImage type is string | undefined because myPath could be null. So to use myImage I will need to add a condition.

? string
: null;

export const getImageAbsoluteURI = <T extends string | null | undefined>(
imageUrl: T,
): ImageAbsoluteURI<T> => {
if (!imageUrl) {
return null;
return null as ImageAbsoluteURI<T>;
}

if (imageUrl?.startsWith('https:') || imageUrl?.startsWith('http:')) {
return imageUrl;
return imageUrl as ImageAbsoluteURI<T>;
}

const serverFilesUrl = REACT_APP_SERVER_BASE_URL;
const serverFilesUrl = new URL(REACT_APP_SERVER_BASE_URL);

serverFilesUrl.pathname = `/files/`;
serverFilesUrl.pathname += imageUrl.startsWith('/')
? imageUrl.slice(1)
: imageUrl;

return `${serverFilesUrl}/files/${imageUrl}`;
return serverFilesUrl.toString() as ImageAbsoluteURI<T>;
};
13 changes: 7 additions & 6 deletions packages/twenty-ui/src/theme/constants/ThemeCommon.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { GRAY_SCALE } from './GrayScale';
import { ICON } from './Icon';
import { MODAL } from './Modal';
import { TEXT } from './Text';
import { Spacing } from '@ui/theme';

export const THEME_COMMON = {
color: COLOR,
Expand All @@ -15,14 +16,14 @@ export const THEME_COMMON = {
animation: ANIMATION,
spacingMultiplicator: 4,
spacing: (...args: number[]) =>
args.map((multiplicator) => `${multiplicator * 4}px`).join(' '),
betweenSiblingsGap: `2px`,
args.map((multiplicator) => `${multiplicator * 4}px`).join(' ') as Spacing,
betweenSiblingsGap: `2px` as Spacing,
table: {
horizontalCellMargin: '8px',
checkboxColumnWidth: '32px',
horizontalCellPadding: '8px',
horizontalCellMargin: '8px' as Spacing,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I've ever seen us doing this. Why do we need it here? Seems like a pattern we want to be careful introducing, if we do it we probably have to do it everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's beneficial to use this type because it ensures that a prop is a Spacing. This doesn't affect the current usage since Spacing is a string. Therefore, if you use a prop of type string, you can provide a Spacing. However, by opting to use a Spacing, as I did in the components, you ensure that the prop will be a spacing.
Additionally, in the Logo component, this is particularly useful because when using the CSS calc function, you need to define the unit in only one of the parameters.

For example:

  calc((${({ theme }) => theme.spacing(3)} * -1) * ${({ size }) => size} / 48);

This is valid because:

  • theme.spacing(3) * -1 = -12px
  • ${({ size }) => size} = 48
  • 48 = 48

Only one parameter returns a unit, so CSS recognizes it as a px spacing.

With a string type instead of a Spacing type, developers might make mistakes, such as using 2rem. So this kind of improvement should increase the DX and the coherence of the global codebase.

checkboxColumnWidth: '32px' as Spacing,
horizontalCellPadding: '8px' as Spacing,
},
rightDrawerWidth: '500px',
rightDrawerWidth: '500px' as Spacing,
clickableElementBackgroundTransition: 'background 0.1s ease',
lastLayerZIndex: 2147483647,
};
2 changes: 2 additions & 0 deletions packages/twenty-ui/src/theme/types/ThemeType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { THEME_LIGHT } from '../constants/ThemeLight';

export type ThemeType = typeof THEME_LIGHT;

export type Spacing = `${number}px`;
Loading