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

Line height in Typeset component is broken #12786

Open
coderitual opened this issue Oct 15, 2020 · 8 comments
Open

Line height in Typeset component is broken #12786

coderitual opened this issue Oct 15, 2020 · 8 comments

Comments

@coderitual
Copy link

coderitual commented Oct 15, 2020

Describe the bug
Line height in Typeset component is broken. Typeset component should allow to provide lineHeights prop which will be corresponding to fontSizes prop. That way we can create proper presentation of font (we always need to define lineHeight to get the best look from the font for a given fontSizes)

Currently it is set based on size, but there is a bug (no px suffix so the unit is relative by default - for fontSize, number is px by default).

Previously its value was fixed to 1.

926b68f

Expected behavior
Exposed lineHeights prop in Typeset component

Screenshots
image

Code snippets
926b68f

@shilman
Copy link
Member

shilman commented Oct 16, 2020

@jpzwarte looks like this was introduced in #12134. mind taking a look?

@jpzwarte
Copy link
Member

@shilman @coderitual I cannot reproduce this. My code:

<Typeset fontFamily="open-sans" fontSizes={['12px', '14px', '16px']}></Typeset>

Which results in:
Screen Shot 2020-10-19 at 10 44 58

@coderitual Are you specifying font-size without a unit? Afaict that is not valid (unless it is a percentage)? https://developer.mozilla.org/en-US/docs/Web/CSS/font-size

@coderitual
Copy link
Author

@jpzwarte in css font-size without unit is invalid - agree. In react you don't need it as px suffix is added automatically to inline properties which support that unit -> https://reactjs.org/docs/dom-elements.html#style

I also think that setting line-height property to the same value as font size is not correct from design point of view. It would be better to leave it to default value which is normal (and differs depending on font-family).

It would be awesome to have lineHeights prop also exposed as in design systems world, font sizing token is always defined as a pair of size and line-height. That way desingers provide the best looking version of font for given font-size in multiline blocks of text (it's called vertical rythm - https://iamsteve.me/blog/entry/a-guide-to-vertical-rhythm).

@jpzwarte
Copy link
Member

@coderitual Setting line-height to the same value as font-size is meant to fix this bug: #12134

A "workaround" for your problem would be to specify a valid font-size (from a CSS perspective).

Feel free to submit a PR that allows you to customize the line-height. The storybook devs are pretty open to such PRs in my experience :)

@coderitual
Copy link
Author

thanks @jpzwarte :)

@stale
Copy link

stale bot commented Dec 25, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@Ryuno-Ki
Copy link
Contributor

Can we reactivate this?

@stale stale bot removed the inactive label Jun 17, 2021
@casey-lentz
Copy link

Looking forward to this update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants