-
Notifications
You must be signed in to change notification settings - Fork 21
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
[Text] It should be possible to set a line-height for a Text component #632
Comments
Thanks for bringing this up @pauloslund. And very apt to bring it up here in Reactist as an issue, even though it feels like a design decision, but I like having the discussion here out in the open rather than internally in Twist (which is what I would've done for a topic like this, but not anymore). I wonder if the line height could be predetermined, and not something we could customize. Maybe it can vary depending on the text size, but in a predetermined way per each text size. I guess we need to involve the @Doist/design-hero for this one. @Doist/design-hero can you comment on what line height should we use? To be clear, we're talking here about text appearing in the UI of our apps, such as labels, description text, etc. It's the same sort of text elements that we already allow customizing the text size to the predetermined values of To give you an idea, the following screenshots from Twist shows various uses of the |
@gnapse after discussing this with the design team, we think that we should start using the text styles we have defined in our Global Library in Figma. All the styles in the list contain line height values as well and for the most part the "San Francisco" style should work just fine. However, currently Twist uses a mix of values in certain places and therefore the switch isn't as straightforward. It's almost certain that there will be places where we'll need to iterate or even create new styles. Therefore it is crucial that we somehow test everything before releasing (perhaps using feature flags?). With the above in mind, how can I help to make this change and the handover of the new styles easier? Is the Figma file enough for you to extract the information needed? |
Unfortunately, setting this up behind a feature flag will be tricky. I can offer this: I could set up a special build of Twist on staging with a version of Reactist in place that would feature the change (that is, with the text elements using the line heights as specified in that Figma library you linked to). That way you folks can use Twist staging during a few days to validate that everything looks good, or any adjustments that we may need to do. Does that sound good? |
@gnapse Given we're using Reactist in Todoist now as well, we would also need to setup a Todoist staging environment right? An idea I had is maybe we could use a feature flag to change the CSS custom properties. For example if we had this code in Reactist: .size-caption {
font-size: var(--reactist-font-size-caption);
line-height: var(--reactist-line-height-caption);
}
.size-copy {
font-size: var(--reactist-font-size-copy);
line-height: var(--reactist-line-height-copy);
}
.size-subtitle {
font-size: var(--reactist-font-size-subtitle);
line-height: var(--reactist-line-height-subtitle);
} We could change the top level value of Let me know if what I'm suggesting isn't clear! |
@gnapse testing everything on a staging environment could work too. 👍 |
Paul, that's all super clear. And yes, TD could be affected too, good point. Anyway, about the approach you suggest, that could work. I was hoping to avoid introducing more feature flags and involving to have to do TD or TW releases. I'm still inclined to validate things on staging first. |
Hey @gnapse, I believe in your Doistalk last week you mentioned that there are a few obstacles in implementing the typography values we have on Figma to the actual product. So I wanted to reach out and ask a) what's the status of this and b) whether there is something we (designers) could do to assist in this transition. Aligning the Web app with our Figma libraries will bring many benefits to the product and our future mockups, hence why we'd love to find a way to move this forward. 😊 |
@tsamoudakis thanks for the follow-up. I do not recall having expressed any concerns about the feasibility to go forward with any typography styles in the short term. Maybe I was misunderstood, but on this particular topic I see little hurdles ahead, or none at all. It's just a matter of making the time. Right now, in this particular issue, the ball is in our court. We'll ping you when we have the situation ready to try, either behind a feature flag, or on staging in Twist and Todoist for us to try before releasing. |
Relevant discussion: https://twist.com/a/1585/ch/1293/t/5845585/ |
🚀 Feature request
It's currently not possible to add a line-height value to a Text component without using
exceptionallySetClassName
. I think it would be help to have alineHeight
property added so that consumers can control the leading of their typography.The values for the line height would need to be determined.
Motivation
The motivation comes from needing to add line height to copy rendered within an application.
Example
This feature could be used any time you want to use a Text component to render multiline copy.
Possible implementations
lineHeight
property could be an absolute value.lineHeight
property could work as a "multiplier" on thesize
prop.The text was updated successfully, but these errors were encountered: