Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

'text-style'/'variant' prop in Maker Text / Maker Heading #269

Closed
bashunaimiroy opened this issue Mar 28, 2022 · 6 comments
Closed

'text-style'/'variant' prop in Maker Text / Maker Heading #269

bashunaimiroy opened this issue Mar 28, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@bashunaimiroy
Copy link
Contributor

bashunaimiroy commented Mar 28, 2022

Feature request

What

A prop in the Maker Text / Maker Heading components, called text-style, variant, text-type, or any name that would be most clear (probably should avoid something conflated with a CSS property like font-style, let's discuss)

Prop Values: The value would refer to one of the four Text Styles(Figma link) e.g. "headline", "title", "body", "label" that have a Font Weight and Font Family set via the Editor.

Behaviour: If we pass one of these values in the text-style prop, e.g.

<m-text text-style="headline"> headline here </m-text>

the MText component will internally resolve a Font Family and Font Weight for the passed value. In this case, Headline Font Family and Headline Font Weight.

These properties would be resolved within MText/MHeading using the MTheme object, similarly to how they're being resolved now (github link). e.g.

...resolveThemeableProps(this.textStyle, ['size', 'fontFamily', 'fontWeight', 'color']),

^ this is a draft, size and color will probably be resolved differently.

Why?

Background

The ECOM Website app is moving towards offering sellers four Text Styles to customise. eg:

  • Headline
  • Title
  • Body
  • Label

The seller will customise these in the Global Font Settings panel (figma link) with a font family and font weight for each Text Style.

So far, we've only discussed how Text Styles apply to Section Elements that are customisable via the options, e.g. a Title Element in a Banner. In this case, the seller would assign that element a Text Style in the Editor Sidebar (figma link), e.g. "Headline" or "Title" as they see fit. This element then takes on the Family and Weight that was set in the Global Settings. Important note: These are usually rendered via the Makerized Text Component (Coda link) which wraps an MText/MHeading component.

Another application of Text Styles is to the non-custom text elements. There are many Text elements that are not customisable via options. For example, Cart Item Title text (Github link). Since we're converting these directly to MText/MHeading too (Coda Link), we could also assign a Text Style- for example Cart Item Titles might be Body or Title.

In both of these cases, we'll need some way to render the seller-set Family and Weight values for each Text Style in the MText/MHeading component. I'm suggesting this new prop.

Alternatives

@pretzelhammer has pointed out we could also achieve this same functionality by simply passing font-family and font-weight props, e.g.

<m-text font-weight="var(--headline-font-weight)" font-family="var(--headline-font-family)"> headline here </m-text>

We'd need to update the fontWeight prop validator to accept vars (github link) but this would achieve the same effect, since these CSS vars will also contain the values of the Headline Font Family/Weight, and MText components will probably be in the scope of these CSS vars.

The advantage of this new prop is:

  1. Slightly more concise- a section developer can say "this is a headline" or "this is a label", set the prop and be done. Future devs can then read it quicker, too- "ah, this is a MText Headline element".
  2. Easier to extend text-styles in the future- if we decide to have more custom style options for each Text Style, for example scale or line-height, we don't have to then add another attribute to every element to specify line-height="var(--headline-line-height).
  3. Keeps the font-family and font-weight props to be used as overrides. e.g. <m-text text-style="headline" font-weight="900"> headline here </m-text> creates a 900-weight headline element.
  4. Keeps us from duplicating logic- TextComponentWithMaker will also have to accommodate a text style prop from the Options. It surrounds MText and MHeading, and would have to calculate the necessary prop values to pass to MText/MHeading, e.g. ${this.textStyle}-font-weight etc. If we add this prop to MText/MHeading, we can simply bind it straight through in TextComponentWithMaker, and reuse the prop resolving logic within MText/MHeading (github link) to resolve the styles for the passed textStyle.

Additional context

Maker Conversion release notes in Coda

@landondurnan
Copy link
Contributor

  1. Considering the type of props we have on other components that change the visual style and where we'd like to go with theming, I believe a variant prop would be the most conducive towards our goals.
  2. We have been discussing internally the fact that the differences between the Heading and Text component are becoming even more vague as the existing props and default style for both have essentially just been a way to set a default font family and weight.

Due to both of these, I think we should look at deprecating the heading component and adding variant to the Text component. The variants can support the four different text styles.

However, I would like to suggest that the names of those variants be a bit different. Specifically I think we should define our two currently supported text and heading options based on the Theme UI Spec in typography with body and heading respectively. And for the other two options, doing a bit more research around what may be the most appropriate names that provide the most meaning. Right now, Headline / Display could work for the larger more graphic option. And Label / UI could work for the fourth style. Granted the names we have in the theme spec and what's supported in Maker do not need to be identical to options in product.

@landondurnan landondurnan added enhancement New feature or request and removed feature request pending triage labels Mar 28, 2022
@pretzelhammer
Copy link
Collaborator

would this "variant" be independent of the "size" prop or not? would the only variants be: headline, title, body, label OR would they be: headline-1, headline-2, ..., label-1, label-2, etc.

@bashunaimiroy
Copy link
Contributor Author

bashunaimiroy commented Mar 29, 2022

would this "variant" be independent of the "size" prop or not? would the only variants be: headline, title, body, label OR would they be: headline-1, headline-2, ..., label-1, label-2, etc.

fair question. Yes, independent- e.g. <m-heading variant="headline" size="7">. Variant is separate from size. Size prop is absolute, not relative to variant.

Context: When we have a Text Variant Size e.g. 'headline-1' it's because the seller chose that size from an Editor Option dropdown. That size gets passed into TextWithMaker's fontSize prop and then makerSize property(github link), which maps it to a Maker Size integer, in this case Size 7 (view font size mapping here).

Finally, it renders an MHeading component with variant="headline" and :size="7"

This mapping process is just for text that's customisable in the Editor Options. For MText elements with static values, we don't have to think about mapping, we just add a variant and absolute size.

@pretzelhammer
Copy link
Collaborator

@bashunaimiroy what should have precedence? should the variant override fontFamily and fontWeight, or should those override the variant?

wait, nevermind, it should go in order of specificity, and since fontFamily and fontWeight are more specific those should always override the variant

@pretzelhammer
Copy link
Collaborator

#278

@landondurnan
Copy link
Contributor

Solution released in v11

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants