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.

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

Feature / CLOUD-24320 / add body text typography #18

Merged
merged 5 commits into from
Aug 7, 2024

Conversation

oli-hivemq
Copy link
Collaborator

  • paragraph
  • label
  • button
  • metric

CleanShot 2024-08-06 at 09 51 37

Kanby

@oli-hivemq oli-hivemq added the enhancement New feature or request label Aug 6, 2024
@oli-hivemq oli-hivemq self-assigned this Aug 6, 2024
@oli-hivemq oli-hivemq marked this pull request as draft August 6, 2024 08:38
@oli-hivemq oli-hivemq force-pushed the feature/CLOUD-24320/typography-text-body-label branch from 9f2cf6c to 886f8a7 Compare August 6, 2024 09:16
@oli-hivemq oli-hivemq marked this pull request as ready for review August 6, 2024 09:17
Comment on lines +63 to +102
P1: {
fontSize: 'P1',
lineHeight: '1.75rem',
},
P2: {
fontSize: 'P2',
lineHeight: '1.5rem',
},
P3: {
fontSize: 'P3',
lineHeight: '1.25rem',
},
P4: {
fontSize: 'P4',
lineHeight: '1rem',
},
P5: {
fontSize: 'P5',
lineHeight: '1rem',
},
L1: defineTypographyStyle('L1', l1Settings, { textTransform: 'uppercase' }),
L2: defineTypographyStyle('L2', l2Settings, { textTransform: 'uppercase' }),
B1: defineTypographyStyle('B1', b1Settings),
B2: defineTypographyStyle('B2', b2Settings),
M1: defineTypographyStyle('M1', m1Settings, { fontFamily: metricsFont }),
M2: defineTypographyStyle('M2', m2Settings, { fontFamily: metricsFont }),
}

export const fontSizes = {
P1: '1.125rem', // can be referenced from sizes/<sizeName>/fontSize e.g. sizes/P1/fontSize
P2: '1rem',
P3: '0.875rem',
P4: '0.75rem',
P5: '0.625rem',
L1: `${l1Settings.fontSizeRem}rem`,
L2: `${l2Settings.fontSizeRem}rem`,
B1: `${b1Settings.fontSizeRem}rem`,
B2: `${b2Settings.fontSizeRem}rem`,
M1: `${m1Settings.fontSizeRem}rem`,
M2: `${m2Settings.fontSizeRem}rem`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

question(non-blocking): I'm thinking if we should write it out as

  • paragraph1
  • paragraph2

Since it might overlap with the padding attribute and then code becomes very hard to read.

Example

<Text variant="P2" p="2" />

Thats just a thought. What do you think @oli-hivemq?

Suggested change
P1: {
fontSize: 'P1',
lineHeight: '1.75rem',
},
P2: {
fontSize: 'P2',
lineHeight: '1.5rem',
},
P3: {
fontSize: 'P3',
lineHeight: '1.25rem',
},
P4: {
fontSize: 'P4',
lineHeight: '1rem',
},
P5: {
fontSize: 'P5',
lineHeight: '1rem',
},
L1: defineTypographyStyle('L1', l1Settings, { textTransform: 'uppercase' }),
L2: defineTypographyStyle('L2', l2Settings, { textTransform: 'uppercase' }),
B1: defineTypographyStyle('B1', b1Settings),
B2: defineTypographyStyle('B2', b2Settings),
M1: defineTypographyStyle('M1', m1Settings, { fontFamily: metricsFont }),
M2: defineTypographyStyle('M2', m2Settings, { fontFamily: metricsFont }),
}
export const fontSizes = {
P1: '1.125rem', // can be referenced from sizes/<sizeName>/fontSize e.g. sizes/P1/fontSize
P2: '1rem',
P3: '0.875rem',
P4: '0.75rem',
P5: '0.625rem',
L1: `${l1Settings.fontSizeRem}rem`,
L2: `${l2Settings.fontSizeRem}rem`,
B1: `${b1Settings.fontSizeRem}rem`,
B2: `${b2Settings.fontSizeRem}rem`,
M1: `${m1Settings.fontSizeRem}rem`,
M2: `${m2Settings.fontSizeRem}rem`,
P1: {
fontSize: 'P1',
lineHeight: '1.75rem',
},
P2: {
fontSize: 'P2',
lineHeight: '1.5rem',
},
P3: {
fontSize: 'P3',
lineHeight: '1.25rem',
},
P4: {
fontSize: 'P4',
lineHeight: '1rem',
},
P5: {
fontSize: 'P5',
lineHeight: '1rem',
},
L1: defineTypographyStyle('L1', l1Settings, { textTransform: 'uppercase' }),
L2: defineTypographyStyle('L2', l2Settings, { textTransform: 'uppercase' }),
B1: defineTypographyStyle('B1', b1Settings),
B2: defineTypographyStyle('B2', b2Settings),
M1: defineTypographyStyle('M1', m1Settings, { fontFamily: metricsFont }),
M2: defineTypographyStyle('M2', m2Settings, { fontFamily: metricsFont }),
}
export const fontSizes = {
P1: '1.125rem', // can be referenced from sizes/<sizeName>/fontSize e.g. sizes/P1/fontSize
P2: '1rem',
P3: '0.875rem',
P4: '0.75rem',
P5: '0.625rem',
L1: `${l1Settings.fontSizeRem}rem`,
L2: `${l2Settings.fontSizeRem}rem`,
B1: `${b1Settings.fontSizeRem}rem`,
B2: `${b2Settings.fontSizeRem}rem`,
M1: `${m1Settings.fontSizeRem}rem`,
M2: `${m2Settings.fontSizeRem}rem`,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point. It was named this way to match what Eric set as text styles on Figma.

CleanShot 2024-08-06 at 12 03 35

On one hand, P1 can be confusing because p could stand for padding.
On another hand, as we're using H1 to H5, the P[1-5], B1|B2, L1|L2, M1|M2 naming doesn't seem that strange.

@tolumq @RobinAtherton @vanch3d @antpaw what's your take on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

True. I was also thinking about h1..6 as it has the same naming pattern. I guess since it's TypeScript it should also be fine as they where named initially.

Copy link

Choose a reason for hiding this comment

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

I'm not sure I know what D is, or B or L or M, let alone with the number beside. It won't make a consistent DevX (why P has 5 but B only 2?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm using shirt size all over the place, now since this is deprecated, all other solutions are okay with me.
I just need "good" typescript support, will I get an error if i do <Button variant="b3" /> ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I know what D is, or B or L or M...

Seems like a good argument for using more than just a letter.

...let alone with the number beside.

Good point, for buttons, labels and "metrics" we could consider going with a more explicit naming convention, like "label small" vs "label default" (making the assumption here that default might be easier to use than medium, because how would you know which one is the default?)

(why P has 5 but B only 2?)

The general idea is to limit the options until we realise we need more. Having said that, I'm unsure we really need 5 P. Eric might have some ideas about the Display options too, but I don't see how we'd use them in the UI. Or maybe they're for a splash screen 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using shirt size all over the place, now since this is deprecated, all other solutions are okay with me. I just need "good" typescript support, will I get an error if i do <Button variant="b3" /> ?

Good idea, looking into that

Copy link

Choose a reason for hiding this comment

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

I was wondering why we are dropping the "native" trousers (😄) size of chakra UI

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding the "native" trousers sizes of Chakra UI:

  1. It comes from a discovery that fontSize is - as it says - only about font size. Wherever we want to alter letter-spacing or text-transform, we need something more than fontSize -> size or variant became the only options for button, label and metrics styles.
  2. I went in this direction as a discussion starter, to see what you guys thought. That objective has been reached 😅 as we have some push back. That's not a bad thing, it means we need to improve how we collaborate and take in developer experience right from the beginning, when creating the design system.

Now it makes us revisit our approach. The answer to this question could become: "ok instead of p1 to p5 for paragraphs, maybe we should update the design system to use the Chakra sizes 🤷‍♂️
Let's discuss in the guild meeting.

Copy link
Collaborator

@h2xd h2xd left a comment

Choose a reason for hiding this comment

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

LGTM!

@oli-hivemq
Copy link
Collaborator Author

In the spirit of moving fast, I will:

  1. merge this PR
  2. add a task to look into typescript support as suggested. I saw that the right typings are getting generated already in the theme's dist. However in the testing pages I don't see that support so it needs looking into. And I also want to check if this support works from a project that includes this lib.
  3. add an item to the agenda of the next guild biweekly to discuss all the points Nicolas brought up on the naming and devX

@oli-hivemq oli-hivemq merged commit a49db43 into main Aug 7, 2024
4 checks passed
@oli-hivemq oli-hivemq deleted the feature/CLOUD-24320/typography-text-body-label branch August 7, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants