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

feat(Paragraph): Add variant prop with long and short #1636

Merged
merged 14 commits into from
Mar 18, 2024
Merged

Conversation

Barsnes
Copy link
Member

@Barsnes Barsnes commented Mar 4, 2024

Resolves #1476

This PR deprecates short, and users will now use variant to set short or long.

short will be removed in #1702

@Barsnes Barsnes marked this pull request as ready for review March 4, 2024 13:52
@Barsnes Barsnes requested a review from mimarz as a code owner March 4, 2024 13:52
Copy link
Contributor

github-actions bot commented Mar 4, 2024

Preview deployments for this pull request:

📖 Storybook 18. Mar 2024 - 10:43 (Norwegian time)

🖥 Storefront 18. Mar 2024 - 10:44 (Norwegian time)

See all deployments at https://dev.designsystemet.no

@Barsnes Barsnes marked this pull request as draft March 4, 2024 14:02
@Barsnes Barsnes marked this pull request as ready for review March 11, 2024 09:49
@mimarz
Copy link
Collaborator

mimarz commented Mar 14, 2024

Hmm, curious if we should just introduce a lineHeight prop with default, short, long as options and just mark short for deprecation. Would make it more clear they are mutually exclusive. What do you think @Barsnes ?

@Barsnes
Copy link
Member Author

Barsnes commented Mar 14, 2024

Hmm, curious if we should just introduce a lineHeight prop with default, short, long as options and just mark short for deprecation. Would make it more clear they are mutually exclusive. What do you think @Barsnes ?

I don't think we should, since line-height is a prop in css, users might think they can send any value in here.
variant, or something along those lines would be better, I think 🤔

@mimarz
Copy link
Collaborator

mimarz commented Mar 14, 2024

Hmm, curious if we should just introduce a lineHeight prop with default, short, long as options and just mark short for deprecation. Would make it more clear they are mutually exclusive. What do you think @Barsnes ?

I don't think we should, since line-height is a prop in css, users might think they can send any value in here. variant, or something along those lines would be better, I think 🤔

hmm, we already have weight on Label which kinda acts the same, expect its just a suggested subset of the actual values for font-weight 🤔

I kinda dont think we should use variant for this as its what we normaly use to denote primary, secondary, tertiary variants for our components.

Hmm, any other suggestions for names? Or we just keep it long and short...

@Barsnes
Copy link
Member Author

Barsnes commented Mar 14, 2024

Hmm, curious if we should just introduce a lineHeight prop with default, short, long as options and just mark short for deprecation. Would make it more clear they are mutually exclusive. What do you think @Barsnes ?

I don't think we should, since line-height is a prop in css, users might think they can send any value in here. variant, or something along those lines would be better, I think 🤔

hmm, we already have weight on Label which kinda acts the same, expect its just a suggested subset of the actual values for font-weight 🤔

I kinda dont think we should use variant for this as its what we normaly use to denote primary, secondary, tertiary variants for our components.

Hmm, any other suggestions for names? Or we just keep it long and short...

If we can't think of anything better I suggest we keep it as long and short - and we can look into this further later 😄

@mimarz
Copy link
Collaborator

mimarz commented Mar 15, 2024

Hmm, curious if we should just introduce a lineHeight prop with default, short, long as options and just mark short for deprecation. Would make it more clear they are mutually exclusive. What do you think @Barsnes ?

I don't think we should, since line-height is a prop in css, users might think they can send any value in here. variant, or something along those lines would be better, I think 🤔

hmm, we already have weight on Label which kinda acts the same, expect its just a suggested subset of the actual values for font-weight 🤔
I kinda dont think we should use variant for this as its what we normaly use to denote primary, secondary, tertiary variants for our components.
Hmm, any other suggestions for names? Or we just keep it long and short...

If we can't think of anything better I suggest we keep it as long and short - and we can look into this further later 😄

variant is also used in Figma so what the heck, lets use that. Better than having two props which are mutually exclusive.

@Barsnes Barsnes changed the title feat(Paragraph): Add long type feat(Paragraph): Add variant prop with long and short Mar 15, 2024
short?: boolean;
/** Variant of the paragraph */
variant?: 'long' | 'short';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we maybe add default so it matches the behaviour of our other variant props? It won't do anything but would make life easier for projects like Studio where user can select these props. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

hm not sure 🤔 It won't hurt to add it, since it does not do anything - your call 😃

Copy link
Collaborator

Choose a reason for hiding this comment

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

We decided to go for variant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In reality we have three different line height. We didn't add a default option in variant as the "default" line-height is backed into the variable used for font and it might be changed with static typography. We will add the third option later when the dust has settled for that. #1707

Copy link
Collaborator

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Added an issue so we remember to the "missing"/default line-height in the future #1707

@Barsnes Barsnes merged commit efedb27 into main Mar 18, 2024
6 checks passed
@Barsnes Barsnes deleted the paragraph/long branch March 18, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add long support to Paragraph (170%)
3 participants