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

What is recommendation for body modifications #17

Open
ai opened this issue Feb 26, 2023 · 4 comments
Open

What is recommendation for body modifications #17

ai opened this issue Feb 26, 2023 · 4 comments

Comments

@ai
Copy link

ai commented Feb 26, 2023

Sometimes I code like this:

.block {
}
body.is-guest-mode .block {
}

For me, it is like @media and I write it by CSS nesting:

.block {
  :is(body.is-guest-mode) & {
  }
}

Here is a real world example.

Should we allow it in nesting rule?

@romainmenke
Copy link
Member

romainmenke commented Feb 26, 2023

Current status.

Source CSS :

.block {
  :is(body.is-guest-mode) & {}
}

This is invalid (in this rule) because it doesn't start with &.

The autofix option can handle this case and transforms to :

.block {
	&:is(:is(body.is-guest-mode) &) {}
}

This is overly complicated and too verbose.

Simpler source :

.block {
	body.is-guest-mode & {}
	body.is-guest-mode * {}
	body.is-guest-mode .block {}
}

autofixed to :

.block {
	&:is(body.is-guest-mode &) {} /* still a warning */
	&:is(body.is-guest-mode *) {}
	&:is(body.is-guest-mode .block) {}
}

This is where you notice that the stylelint plugin doesn't try to interpret your selector.
It will only try to fix it so that the warning goes away by placing an & at the start.


Some thoughts :

&:is(body.is-guest-mode &) this is a warning now but I actually think this is ok.
Maybe best to drop this restriction and allow multiple &.

If I recall correctly I mostly wanted to avoid this :

.foo {
  :is(&:does-not-exist, .bar) {}
}

In this example .bar is a standalone selector. It isn't prefixed with .foo in any way. It is not .foo .bar as you might expect.

But this pattern is made invalid by requiring a top level &.


body.is-guest-mode &

The current rule says :

  • & must be first
  • & must be followed by a single pseudo

Maybe better to actually do this :

  • only the last compound selector is validated
  • last compound must start with &
  • last compound may contain pseudo classes

This relaxed version does bring back a lot of complexity:

  • relative selector syntax
  • starts with symbol vs. starts with ident

But it doesn't break the core principle of the coding style.

I don't know if this relaxed version can have sane autofixing and I really like the current autofixing.

@romainmenke
Copy link
Member

v2.1.0 contains a few relevant fixes :

  • multiple & are now allowed : .foo { &:is(& + &) {} }
  • better autofix for body.is-guest-mode &
.block {
	body.is-guest-mode & {}
}

becomes :

.block {
	&:is(body.is-guest-mode *) {}
}

This is a better autofix because it preserves the specificity of the original and matches the same elements.


This is still not allowed by this rule :

.block {
  :is(body.is-guest-mode) & {
  }
}

For now I want to try the currently allowed patterns.
If it is too annoying I might relax it in the future.

@ai
Copy link
Author

ai commented Feb 28, 2023

  1. What is your current code style for selectors like body.is-guest-mode .block? How do you write these selectors?
  2. Are you sure that block { body.is-guest-mode & {} } is equal to .block { &:is(body.is-guest-mode *) {} }? The auto-fix looks like a completely different selector.

@romainmenke
Copy link
Member

What is your current code style for selectors like body.is-guest-mode .block? How do you write these selectors?

We (at Mr. Henry) don't write any nested CSS today because we only use features that are implemented in at least two engines.

In the past we did write nested CSS but this made code reviews much harder.
With nesting shipping soon we want to have some guard rails in place to make sure that code remains review-able.

Because we are starting from a point where none of our CSS is nested it's easier for us to start with a really strict coding style and relax later where needed.

If we start with something too lax and discover that nesting is harming code reviews again we will need to spend time on refactoring past work.

Long way of saying : we don't have a current code style for body.is-guest-mode .block in nested CSS today. We need to try it out.

I really like the pseudo's (:focus, :is(), :has()...) for the simple cases.
You can read these selectors like a sentence :

.foo {
  /* foo when it has an image */
  &:has(img) {}

  /* foo when it is also bar */
  &:is(.bar) {}
}

But we need to try this out over the coming months to have a better feel for what works well and what doesn't.


Are you sure that block { body.is-guest-mode & {} } is equal to .block { &:is(body.is-guest-mode *) {} }? The auto-fix looks like a completely different selector.

Yes, these are the same :)

.block { &:is(body.is-guest-mode *) {} }

:is() desugaring ->

.block { body.is-guest-mode *& {} }

& desugaring ->

.block:is(body.is-guest-mode *) {}

both desugared ->

body.is-guest-mode *.block {}

:is() with complex selectors does look completely different because the bits seem out of order. It is the last compound that corresponds with .block, in this case that is *.

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

No branches or pull requests

2 participants