generoi / genero-design-system

https://gds.generogrowth.com/
MIT License
4 stars 0 forks source link

maybe redundant mixin naming? #25

Closed silentnoodlemaster closed 4 years ago

silentnoodlemaster commented 4 years ago

https://github.com/generoi/genero-design-system/blob/c943c25b9e58b23061f30674ba15588cc780b818/src/components/gds-paragraph/_index.scss#L1-L24 here we define the paragraph-l mixing that I would then use as:

@use '~genero-design-system/src/components/gds-paragraph' as paragraph;

p {
  @include paragraph.base;

  &.has-large-font-size {
    @include paragraph.paragraph-l;
  }
}

feels a little silly having paragraph.paragraph-l

I think this would nice but does it cause scoping issues, i would assume that it doesn't since we have adopted the @use keyword:

@mixin size-l { . . . }

thoughts? specifically @taromorimoto and @oxyc could weigh in

silentnoodlemaster commented 4 years ago

btw @use ' . . . ' as * would make it not work if you need many components in one file. for the base would conflict already

taromorimoto commented 4 years ago

@silentnoodlemaster can you tell me where does this .has-large-font-size class name come from? Is it that some standard WP way to have class names relative to the base class? You see different size of components might have some other changes than just font-size.

Where can I find all class names WP uses as "standard"? You see, it would help me when I'm implementing the GDS components to try to make them match the WP equivalents.

taromorimoto commented 4 years ago

What's the idea behind this @mixin size-l { . . . }? It seems very generic and I feel that it's better to have more specific classes so that changes to one thing wouldn't eventually break others? Basically it means less dependencies between styles.

silentnoodlemaster commented 4 years ago

i usually bust create the block in wp and inspect element so don't know where you would find all of them. @oxyc you know?

if the mixin was called size-l then you would @include paragraph.size-l or even @include heading.size-l. the word left of the period being what you put after as when you use @use

makes sense?

oxyc commented 4 years ago

@silentnoodlemaster can you tell me where does this .has-large-font-size class name come from? Is it that some standard WP way to have class names relative to the base class? You see different size of components might have some other changes than just font-size.

Yeah that's WPs way. It's a built-in in Gutenberg that all paragraphs and headings get. They use .has-large-font-size, .has-text-center-aligned, .has-primary-background-color etc.. but it shouldn't really matter because we anyways cant use Web Components for these primitive Gutenberg blocks. For our own block we could always map it straight to web component properties.

What's the idea behind this @mixin size-l { . . . }? It seems very generic and I feel that it's better to have more specific classes so that changes to one thing wouldn't eventually break others? Basically it means less dependencies between styles.

Same as what @silentnoodlemaster said. Now with @use we can namespace them so we dont need to have it in the naming. I do perfer size-l as well. It's sort of like when PHP added namespaces :D

taromorimoto commented 4 years ago

@oxyc @silentnoodlemaster yeah, makes sense. I'll start using namespaced naming.