liferay / clay

A web implementation of the Lexicon Experience Language
http://clayui.com
Other
208 stars 483 forks source link

Clay and Lexicon should have the same type scale #4604

Closed claraizquierdo closed 2 years ago

claraizquierdo commented 2 years ago

What is your proposal?

It would be nice that Clay implements all the typography sizes described in Lexicon (you can see them here). In Clay we have these typography classes available (six headings + text inline): https://clayui.com/docs/css/content/typography.html . Also, the sizes available in Clay do not match with the sizes defined in Lexicon, for example, h1 size is defined in Lexicon with a font-size: 26px, but in Lexicon I don't see anything with that size.

Why would adopting this proposal be beneficial?

If this is implemented, we wouldn't need to add custom font-size or line-height in our code. We would have less CSS and if Clay changes the typography sizes, they would be changed in all the portal. It would be easier for developers to add styles to our components.

What are the alternatives to this proposal?

Lexicon could be adapted to what we have already developed in Clay.

julien commented 2 years ago

Hey @claraizquierdo thanks for opening this.

I personally don't think this issue is only related to Clay, but it might be a good opportunity to talk about the discrepancies that exist between Clay/Lexicon/D.X.P.

What I've observed (and it's not the first time) is that some things are defined in Lexicon but aren't defined in Clay, and some things are defined in D.X.P. that don't exist either in Lexicon or in Clay.

To give you some context, we are 3 in the Clay squad, and the only one working on "markup and styles" is @pat270 (so that's just one person).

When a designer asks a developer to "use something from Lexicon", they should make sure it exists in Clay - otherwise it creates a lot of confusion, especially for new people.

We should also make sure that everything we define (both in Clay and Lexicon) is going to be used in D.X.P. otherwise, we're just applying the YAGNI principle, and I don't think that's the goal (but I might be wrong).

I personally think that we should stop thinking that both Clay and Lexicon are going to solve every problem (we don't have the time or the resources for that), and we need to make sure Lexicon and Clay can be used easily in D.X.P. because that's the only reason Lexicon and Clay exist in the first place (I know that's a bold statement and I hope I won't offend anyone by expressing my thoughts).

With that said, I think this issue is totally valid and that we should fix it, but it would be great if we found a way to avoid this kind of problem in the future: that's why I'm adding @victorvalle, @drakonux and @matuzalemsteles (as well as @dsanz, because I want him to be aware of this recurring issue) to this "thread" in order to discuss the problem and see how we can better coordinate.

pat270 commented 2 years ago

@claraizquierdo The reason we don't have utility classes for Lexicon's type scale is to prevent random use of font size utilities throughout DXP components. This actually applies to most utility classes. The reason Clay is so anti utility is that OOTB markup in DXP is hard to change for our users.

Utility classes make it difficult to make CSS changes and is compounded by the use of !important. We decided to go with a component based approach for our CSS to help overcome this DXP limitation. This means that if you need font-size: 10px, define it in the component not in a utility class with no meaning.

Lastly, if this is something we must have, we might be able to add it to Clay Admin. Admin only components are hard to modify by definition, I would have no problems doing what we want there.

julien commented 2 years ago

@pat270 thanks for the quick reply. I also wanted to make sure this has been agreed with @liferay-lexicon because it seems that there are still some doubts - you can read more about that here (Internal Slack Thread)

claraizquierdo commented 2 years ago

@pat270 @julien thank you for your answer. Everything is a bit clearer now. For me, as a developer in the Tango Team, it would be very helpful to have this utility classes or variables in Clay Admin, bit it's your decision to make. Tell me if I can do something to help :)

drakonux commented 2 years ago

Well, from our side, we only need the following scale:

image

And that's it. We mustn't set how Clay decides to implement the scale. Lexicon should be agnostic to the technology. For example, Lexicon mustn't say what size is an H1 in DXP.

About other information in the lexicon site related to the use of the scale (forms, dividers, etc.) is mainly information pertaining to the usage of the scale in DXP that Lexicon should not cover. Thus, we will update the site removing that information.

matuzalemsteles commented 2 years ago

@drakonux this makes sense, I think on the Clay side we can use the last 6 scales of this definition to define the heading but we can also offer all this scale outside the heading, it makes sense for example that in some places they don't need to use the heading. But we also need to provide a guide on the Clay side for when to use heading vs a scale in a p tag for example in the case of the last ones of the scale.

@pat270 we could use the same pattern that exists in some CSS library or how tailwind does the scale naming for when it is not related to the heading, for example:

Class REM
text-2xs 0.625rem
text-xs 0.75rem
text-sm 0.875rem
text-base 1rem
text-lg 1.125rem
text-xl 1.25rem
text-2xl 1.5rem
text-3xl 1.75rem
text-4xl 2rem
text-5xl 2.25rem
text-6xl 2.5rem

This would also be a great opportunity to insert components like Heading and Text into Clay, I had talked about this last year to do the foundation, I'm glad we can align this on the CSS side.

<Heading level="4">Lorem</Heading>

<Text size="3xl">Lorem</Text>
pat270 commented 2 years ago

I think this is a little early for us to implement. I can see devs creating whole portlets only with utility classes. Unfortunately our infrastructure doesn't allow changing the markup via Display Templates or some other feature for every portlet. Utilities without easy access to the markup is no good. CSS becomes harder because you need to target the specific text-xs inside a specific portlet, nested deeply inside utility classes.

When/if DXP becomes more friendly to this style of development, we should implement it (imo).

I'm open to this in Clay Admin because:

1) Markup is easy to change (for us) 2) Generally, users are not expecting to modify our admin interface

I'm not sure what this means for Clay components (Text, Heading, etc), but if they are admin only sure why not.

it would be very helpful to have this utility classes or variables in Clay Admin

We can add Sass variables to both, but only utility classes to Clay Admin.

matuzalemsteles commented 2 years ago

Yeah, I can see that it can be a problem to apply this to the Portal in general, well I like for example the idea of ​​using this as a default for the components, it's very powerful when you have it, because if a space a size a color changes, this will be consistent for all components, for example, typography is something we don't have in components, so if we change this it doesn't affect the component exclusively. That would be very interesting.

I think we could still offer this option for both sides, the text-* option would just be a possibility of more options compared to what we don't have today, that is, you can achieve the same values ​​without using the heading, so the idea here would be to update the heading values ​​if necessary and provide the rest of the sizes for the utility classes. Because I can't see an option where we would be able to offer all these sizes in the heading.

To complement and make it clearer, the heading updates could be:

Heading REM Class equivalent
h1 2.5rem text-6xl
h2 2.25rem text-5xl
h3 2rem text-4xl
h4 1.75rem text-3xl
h5 1.5rem text-2xl
h6 1.25rem text-xl

That is, the Headings by default have these sizes without having to add classes.

ethib137 commented 2 years ago

And what about Style Books? 😄 It will be great if whatever solution we decide on also leverages Style Books, so that font sizes throughout Liferay can easily be updated. Sass variables and utility classes can do this if they implement css variables that align with our style book tokens. Using css variables would be especially important for the Sass variable approach since our current use of 'atlas-variables' in other Liferay modules does not guarantee consistency with the Liferay theme being used (More Explanation Here).

I think it would be best if we implement Sass variables AND utility classes. It makes sense that we should define a best practice to use Sass variables whenever possible since this should make it easier to restyle, but utility classes are still very helpful for cases when sass is unavailable. A couple examples of this would be custom page fragments or remote apps. In these cases utility classes can provide easy opportunities for consistency, and if we use css variables that align with style books, it would allow this type of custom code to easily inherit from style books too.

pat270 commented 2 years ago

Ok I'm convinced. I'm going to add the font size utilities. Right now I'm leaning toward Bootstrap 5's namespace .fs-1, .fs-2, ..., .fs-#. If -2xs, -xs, -sm, -md, -xl, -2xl is better we can go with that, but I find it harder to remember. I might leave off the !important flag and add it later if we need it. Easier to add than remove.

@matuzalemsteles There was an attempt to update h1-h6 font sizes, but it broke too many web contents that were hard to update or something like that. Maybe we can change it now @marcoscv-work?

@ethib137 You will be able to use CSS variables in the utilities, they won't be based off $font-size-base, $font-size-lg, or $font-size-sm.

matuzalemsteles commented 2 years ago

There was an attempt to update h1-h6 font sizes, but it broke too many web contents that were hard to update or something like that.

Hmm yeah, you're right this is going to cause a big change in DXP and system.

Ok I'm convinced. I'm going to add the font size utilities. Right now I'm leaning toward Bootstrap 5's namespace .fs-1, .fs-2, ..., .fs-#. If -2xs, -xs, -sm, -md, -xl, -2xl is better we can go with that, but I find it harder to remember. I might leave off the !important flag and add it later if we need it. Easier to add than remove.

No problem, whatever you think is best. Yes, fs is a little less intuitive but if it's to follow bootstrap it makes more sense 🙏.

ethib137 commented 2 years ago

There was an attempt to update h1-h6 font sizes, but it broke too many web contents that were hard to update or something like that.

This probably doesn't need to be a big issue now, due to style books. If we allow style books to set the h1-h6 sizes, we can set the default to the Lexicon values. Then if any customer is upgrading to 7.4 and has an issue with the new values, part of the upgrade path can simply be to reset the style book values for h1-h6 to the previous values.

matuzalemsteles commented 2 years ago

I'm going to manually close this issue as we have PR #4616 merged. Feel free to create another issue if you have any further questions or suggestions.

github-actions[bot] commented 2 years ago

This issue has been merged and will be released in DXP at https://issues.liferay.com/browse/LPS-146454