openverse-frontend icon indicating copy to clipboard operation
openverse-frontend copied to clipboard

Group text styles in custom classes

Open krysal opened this issue 3 years ago • 2 comments

Problem

The text styles defined by @panchovm in mockups normally involve more than a font-size and we find ourselves often missing other properties like font-weight and line-height. We can simplify the work of applying these styles correctly (and making future global modifications) by grouping these styles in simple CSS classes or (maybe even adding custom Tailwind utilities?) with the names he has already defined in Figma.

Additional context

This will prevent and solve quickly issues like #1587. Later I found this related issue #1489 but I find overkill to create components for plain headings, paragraphs, labels, etc.

Implementation

  • [ ] 🙋 I would be interested in implementing this feature.

krysal avatar Aug 13 '22 03:08 krysal

Thanks for creating this issue.

I was planning to start working on this and come up with a style update proposal. The current design definitions have some inconsistencies when applied to components and other pieces of text.

To tackle this from the design, I will add the needs design label.

fcoveram avatar Aug 23 '22 20:08 fcoveram

I created this file where I am exploring and testing the styles, but before sharing the approach and some outcomes, I would like to test them online.

How can we mirror the current site and see what components and layouts look like? My first thought was coding them on codepen but for sure there is a better way. It would be great to have a css file or similar and adjust the values to see how everything changes.

Any ideas @krysal?

fcoveram avatar Aug 24 '22 20:08 fcoveram

@panchovm Sorry, I haven't had the chance to look for options. Tagging other folks for any ideas @obulat @dhruvkb @zackkrida

krysal avatar Sep 01 '22 16:09 krysal

Thank you for tagging, @krysal!

The difficulty with this issue is that the heading styles are different on different screen widths. Sometimes, the size changes on the md breakpoint, but sometimes - on lg breakpoint. An even larger complication is that the same sizes on desktop can correspond to different sizes on the mobile. For example, Heading 5 on desktop can correspond to mobile style ### Description bold style (for image title on the single detail page) or to Heading 5 (for "Related images" heading)

I tried creating a functional component for Heading that would take the semantic level and the style level for the heading. Functional - to reduce the amount of overhead and prevent worsening of performance. But I couldn't get it to work with Typescript, and found that the different styles for mobile and desktops made this complicated, too. Besides, sometimes we need text styles for buttons or tabs that are not headings.

So, I tried out the utility styles you linked to, @krysal, and they seem perfect for this! I am going to open a new PR adding utility styles for the styles from Figma designs.

obulat avatar Sep 02 '22 08:09 obulat

I'm glad that the custom utilities worked out for this case 😃 I'm sure we will find situations when we can't use our custom utilities due to difficulties matching styles but I hope those are exceptions more than the norm.

One thing needed here before a PR, is that @panchovm wanted to review (and likely change some of) those styles @obulat, given the final result often looks a bit different on Figma than on the web browsers. Could you coordinate with him to check the changes applied to the components?

krysal avatar Sep 02 '22 14:09 krysal

That sounds excellent @obulat Thanks for trying it. I forgot to link the file where I am testing the styles, but here is.

So far, I have been replicating each view on different breakpoints to see how the text styles change in components. If we reach setting up something like that, that would be perfect.

fcoveram avatar Sep 05 '22 14:09 fcoveram

Do I understand it correctly, @panchovm, that in New styles you marked the styles that were changed with pink and the ones that are no longer used with lighter charcoal color?

obulat avatar Sep 05 '22 14:09 obulat

Yes. But that is still pending. Please do not take it as a final decision.

I wanted to put my hands on some front-end code, but since I will take AFK days soon, perhaps it is worth keeping the Figma test and moving to a simple code test once I have the first outcome.

fcoveram avatar Sep 05 '22 15:09 fcoveram