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

AMoreaux · 2024-11-19T15:18:57Z

This Pull Request adds support for resizing the logo and optionally including the Twenty logo in the component.

Added size and includeTwentyLogo props to the Logo component.
Updated the styled components to handle dynamic sizing based on the new size prop.
Added logic to conditionally render the Twenty logo based on the includeTwentyLogo prop.
Incorporated the useTheme hook to access theme values directly within the component.

Enhanced Logo component to accept dynamic size and an optional twenty logo display. This allows for more flexible and customizable logo dimensions, and the inclusion of Twenty's logo can now be controlled via props.

greptile-apps

PR Summary

Enhanced the Logo component with dynamic sizing and optional Twenty logo display, improving flexibility for authentication UI customization.

Added size prop with theme-based default of theme.spacing(12) in /packages/twenty-front/src/modules/auth/components/Logo.tsx
Implemented dynamic calculations for Twenty logo container dimensions (28/48 of main size) and positioning
Added includeTwentyLogo boolean prop to control Twenty logo visibility
Introduced TypeScript interfaces LogoProps and StyledMainLogoProps for better type safety
Refactored fixed pixel values to use relative sizing based on the size prop

_{1 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

packages/twenty-front/src/modules/auth/components/Logo.tsx

Changed StyledMainLogo type to use Pick for logo prop. This ensures only the required 'logo' property is passed, improving type safety and code clarity.

Consolidated the size prop inside the StyledMainLogo component call. This removes redundancy and aligns with component usage patterns in the codebase.

packages/twenty-front/src/modules/auth/components/Logo.tsx

FelixMalfait · 2024-11-20T16:00:02Z

packages/twenty-front/src/modules/auth/components/Logo.tsx

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


The expression ${({ theme }) => -${theme.spacing(3)}} is equivalent to -12px.

Due to CSS limitations, I initially had to use a raw value like -12. However, with the latest commit, I found a workaround that allows us to avoid using absolute values while still utilizing theme.spacing(...).

Otherwise, it's a proportional calculation.

packages/twenty-front/src/modules/auth/components/Logo.tsx

FelixMalfait · 2024-11-20T16:19:52Z

packages/twenty-front/src/modules/auth/components/Logo.tsx

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


could you handle that through a default prop value? Something feels not very elegant here

Updated the Logo component to accept primary and secondary logos, replacing the workspaceLogo prop. Adjusted the getImageAbsoluteURI utility function for type safety and URL handling.

Use the new `Spacing` type to ensure consistent spacing units across the theme and Logo components. This change streamlines unit handling and improves type safety throughout the codebase.

AMoreaux · 2024-11-20T17:29:51Z

Following a discussion with Thomas, he requested that we swap the twenty logo with the other one.

To accommodate this, I updated the pull request to create a more flexible component by introducing primaryLogo and secondaryLogo properties.

Regarding the size, nothing changes if you retain the default value. However, if you opt for a different size, we need to adjust the logo positions accordingly. This is why you'll notice several proportional calculations.

FelixMalfait · 2024-11-21T13:06:14Z

packages/twenty-ui/src/theme/constants/ThemeCommon.ts

-    horizontalCellMargin: '8px',
-    checkboxColumnWidth: '32px',
-    horizontalCellPadding: '8px',
+    horizontalCellMargin: '8px' as Spacing,


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

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.

Mmhh this is very interesting but I think it requires more work

We should look into
https://emotion.sh/docs/typescript

FelixMalfait · 2024-11-21T13:09:36Z

packages/twenty-front/src/utils/image/getImageAbsoluteURI.ts

@@ -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


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

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.

charlesBochet · 2024-11-22T17:26:56Z

packages/twenty-front/src/modules/auth/components/Logo.tsx

-  workspaceLogo?: string | null;
+  primaryLogo?: string | null;
+  secondaryLogo?: string | null;
+  size?: Spacing | null;


if we were to do that:
size: 'small' | 'medium' | 'large'

I think we should actually have one size

charlesBochet · 2024-11-22T17:27:52Z

packages/twenty-ui/src/theme/constants/ThemeCommon.ts

@@ -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 = {


type THEME_COMMON:

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

c3024f7

Enhanced Logo component to accept dynamic size and an optional twenty logo display. This allows for more flexible and customizable logo dimensions, and the inclusion of Twenty's logo can now be controlled via props.

greptile-apps bot reviewed Nov 19, 2024

View reviewed changes

packages/twenty-front/src/modules/auth/components/Logo.tsx Outdated Show resolved Hide resolved

packages/twenty-front/src/modules/auth/components/Logo.tsx Outdated Show resolved Hide resolved

AMoreaux added 2 commits November 19, 2024 17:06

refactor(Logo): Update StyledMainLogo type definition

c62ef0e

Changed StyledMainLogo type to use Pick for logo prop. This ensures only the required 'logo' property is passed, improving type safety and code clarity.

refactor(Logo): simplify StyledMainLogo component usage

9cbbe51

Consolidated the size prop inside the StyledMainLogo component call. This removes redundancy and aligns with component usage patterns in the codebase.

AMoreaux mentioned this pull request Nov 20, 2024

feat(subdomain): use subdomain by workspace #8378

Closed

8 tasks

FelixMalfait reviewed Nov 20, 2024

View reviewed changes

packages/twenty-front/src/modules/auth/components/Logo.tsx Outdated Show resolved Hide resolved

FelixMalfait reviewed Nov 20, 2024

View reviewed changes

packages/twenty-front/src/modules/auth/components/Logo.tsx Outdated Show resolved Hide resolved

FelixMalfait reviewed Nov 20, 2024

View reviewed changes

AMoreaux added 2 commits November 20, 2024 17:59

refactor(Logo): support primary and secondary logos

bbe4baf

Updated the Logo component to accept primary and secondary logos, replacing the workspaceLogo prop. Adjusted the getImageAbsoluteURI utility function for type safety and URL handling.

refactor: implement Spacing type for consistent units

68580bb

Use the new `Spacing` type to ensure consistent spacing units across the theme and Logo components. This change streamlines unit handling and improves type safety throughout the codebase.

AMoreaux requested a review from FelixMalfait November 20, 2024 17:34

FelixMalfait reviewed Nov 21, 2024

View reviewed changes

AMoreaux requested a review from FelixMalfait November 21, 2024 14:58

Merge branch 'main' into feat/front-improve-component-logo

68ec09a

FelixMalfait self-assigned this Nov 22, 2024

charlesBochet reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AMoreaux commented Nov 19, 2024 •

edited

Loading

greptile-apps bot left a comment

FelixMalfait Nov 20, 2024

AMoreaux Nov 20, 2024

FelixMalfait Nov 20, 2024

AMoreaux commented Nov 20, 2024

FelixMalfait Nov 21, 2024

AMoreaux Nov 21, 2024

FelixMalfait Nov 22, 2024

FelixMalfait Nov 21, 2024

AMoreaux Nov 21, 2024

charlesBochet Nov 22, 2024

charlesBochet Nov 22, 2024

charlesBochet Nov 22, 2024


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

		const includeTwentyLogo = isDefined(props.includeTwentyLogo)

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

Are you sure you want to change the base?

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

Conversation

AMoreaux commented Nov 19, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMoreaux commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMoreaux commented Nov 19, 2024 •

edited

Loading