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鈥檒l 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

Open
pauloslund opened this issue Jan 31, 2022 · 9 comments
Open

Comments

@pauloslund
Copy link
Contributor

pauloslund commented Jan 31, 2022

馃殌 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 a lineHeight 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

  • A lineHeight property could be an absolute value.
.lineHeight-small  { line-height: 1.2; }
.lineHeight-medium { line-height: 1.6; }
.lineHeight-large  { line-height: 2.0; }
  • A lineHeight property could work as a "multiplier" on the size prop.
.size-caption {
    font-size: var(--reactist-font-size-caption);
    line-height: var(--reactist-line-height-caption);
}

.line-height-caption-small {
    line-height: calc(var(--reactist-line-height-caption) - 0.2);
}

.line-height-caption-medium {
    line-height: var(--reactist-line-height-caption);
}

.line-height-caption-large {
    line-height: calc(var(--reactist-line-height-caption) + 0.2);
}
@gnapse
Copy link
Contributor

gnapse commented Jan 31, 2022

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 body, caption, copy and subtitle.

To give you an idea, the following screenshots from Twist shows various uses of the Text component that would be affected by this.

CleanShot 2022-01-31 at 15 20 08@2x

CleanShot 2022-01-31 at 15 21 03@2x

CleanShot 2022-01-31 at 15 21 59@2x

CleanShot 2022-01-31 at 15 23 55@2x

@tsamoudakis
Copy link

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

@gnapse
Copy link
Contributor

gnapse commented Feb 2, 2022

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?

@pauloslund
Copy link
Contributor Author

@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 --reactist-line-height-caption using JS. If the feature flag is disabled we could set it to revert or something similar, otherwise just use the value from Reactist. It might be an alternative to testing in staging environments.

Let me know if what I'm suggesting isn't clear!

@tsamoudakis
Copy link

@gnapse testing everything on a staging environment could work too. 馃憤

@gnapse
Copy link
Contributor

gnapse commented Feb 3, 2022

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.

@tsamoudakis
Copy link

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. 馃槉

@gnapse
Copy link
Contributor

gnapse commented Feb 21, 2022

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

@gnapse gnapse self-assigned this Jan 20, 2023
@gnapse gnapse removed their assignment Oct 16, 2023
@frankieyan
Copy link
Member

Relevant discussion: https://twist.com/a/1585/ch/1293/t/5845585/

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

4 participants