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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Typography prefix and default styles #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucasvallenet
Copy link
Contributor

The typography styles shouldn't be treated as a components or an object but as a different entity. I'm introducing a new concept of how to manage typography styles within a dedicated folder typography and using a new class prefix .t-*. Each different styles should be using this prefix and be located within this folder: .t-text, .t-wysiwyg, .t-link, ...

I'm also updating the heading class: .c-heading.-h1 > .t-h1
This will lighten the html markup and simplify the sass @extends.
The heading font sizes are stored within a css custom property in elements/_page.scss to allow responsive sizes and the use of a heading size for an other component.

I added default styles for wysiwyg fields with margins, title and list styles

@mcaskill
Copy link
Member

mcaskill commented May 2, 2022

Hrm. I don't know what the others think but so far we've been following Harry Robert's CSS namespaces wherein the .t- prefix is reserved for "themes". Maybe we want to move away from that, if so, we should define this in the documentation section.

@Jerek0
Copy link
Member

Jerek0 commented May 2, 2022

Why do you think we shouldn't treat typography styles as a component or an object? I'm not against the idea, but I'm not sure to see the benefits. Could you explain why such a change should happen?

Regarding the themes thing, as far as I recall we never really used that concept in a project - at least since I'm at Locomotive - so it wouldn't bother me to hijack that prefix. @mcaskill is right about defining it in the doc though.

Great idea to use the CSS variables for the heading font sizes 👍

Thanks for the PR @lucasvallenet 🙏

@lucasvallenet
Copy link
Contributor Author

@mcaskill I don't recall using a theme in any website, it's not really common so we could easily find a workaround if necessary.

@Jerek0 In my opinion, a component is a complexe block that can have different children blocks depending on use case, and can also contain objects as children (a c-card_tag | o-tag withing a c-card for example).
An object is more straight forward and should always have the same children html markup, it could be used for an icon or a loader for example. An object shouldn't contain another object, even less a component.
Based on that, the text rules could be an object, but they are usually single elements selectors. It could be treated as a utility class, but I think that it would make sense to have a dedicated prefix for these typographic rules.

Use cases:

<span class="o-tag">
    <span class="o-tag_name | t-h4"></span>
</span>

It's more readable and it would also reduce the use of @extendswithin the sass to avoid duplicated styles within the css and more consistency on the html markup.

That's just an idea and a methodology I use to have, I'm really open for discussion or to drop the pull request no harm done :)

@mcaskill
Copy link
Member

mcaskill commented Oct 5, 2022

Outcome from meeting on 2022-10-05

  • Status: Changes barring approval
  • Revert .t-wysiwyg back to .u-wysiwyg.
  • Exclude .t-link.
  • Approved .t-*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants