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

Add ignoreSelectors: [] to selector-max-compound-selectors #7544

Merged
merged 18 commits into from Mar 13, 2024

Conversation

FloEdelmann
Copy link
Member

@FloEdelmann FloEdelmann commented Feb 26, 2024

Which issue, if any, is this issue related to?

Closes #7536.

Is there anything in the PR that needs further explanation?

I don't think ignoring :not should also ignore contents of it.

Copy link

changeset-bot bot commented Feb 26, 2024

🦋 Changeset detected

Latest commit: 411ed1e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ybiquitous ybiquitous changed the title Add ignoreSelectors option to selector-max-compound-selectors rule Add ignoreSelectors: [] to selector-max-compound-selectors Feb 28, 2024
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request. Can you add the option to the rule's README?

.changeset/big-trains-learn.md Outdated Show resolved Hide resolved
lib/rules/selector-max-compound-selectors/index.mjs Outdated Show resolved Hide resolved
{
code: '.foo { & ::v-deep > .bar {} }',
description: 'nested ignored selector',
},
Copy link
Contributor

@Mouvedia Mouvedia Mar 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question
major

How should '.foo .baz .bar.quz {}' behave for 2 ignore [".bar", ".quz"]?
i.e. does it remove one by one from the compound selector or does it skip it if it's not exactly a match?
Either way is fine with me but it needs to be documented either with a > [!NOTE] or an example in the README.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ybiquitous ybiquitous mentioned this pull request Mar 8, 2024
4 tasks
@Mouvedia
Copy link
Contributor

@ybiquitous should we move on and leave the note for later?
Or push it to the next version?

@ybiquitous
Copy link
Member

@Mouvedia Good point. I think adding a not is a good idea to reduce people's confusion.

In the example you suggested,

{
  "selector-max-compound-selectors": [2, {"ignoreSelectors": [".bar", ".quz"]}]
}
.foo .baz .bar.quz {}

it should be considered as:

.foo .baz {} /* ".bar" and ".quz" are ignored */

because it seems easier to implement by using the current selector parser (postcss-selector-parser).

@Mouvedia
Copy link
Contributor

Whatever the statu quo is, I am fine with it; but it needs to have a pass/fail example or a note.
It's not a blocker so it could be punted to another PR.

@ybiquitous
Copy link
Member

Although it's not a blocker, I think adding a note in this PR is preferable.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mouvedia I think adding a code example instead of a note is enough, but what do you think?

@FloEdelmann I would like to include this PR in the next version. If you don't have time, I'll push additional commits instead.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggest] I found that we can remove the previousNodeWasCompoundSeparator variable outside the each loop by using the index second argument. As a result, we can put the ignoring code in one if condition.

lib/rules/selector-max-compound-selectors/index.mjs Outdated Show resolved Hide resolved
lib/rules/selector-max-compound-selectors/index.mjs Outdated Show resolved Hide resolved
lib/rules/selector-max-compound-selectors/index.mjs Outdated Show resolved Hide resolved
@FloEdelmann
Copy link
Member Author

@ybiquitous Sorry for not responding. I currently don't have much time to work on this PR, so please go ahead and edit to your liking!

@Mouvedia
Copy link
Contributor

@Mouvedia I think adding a code example instead of a note is enough, but what do you think?

Either one is fine.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed additional commits.

@Mouvedia Can you take a look again?

@ybiquitous
Copy link
Member

I bumped postcss-selector-parser and removed the patch (91c2a8e) since the new version has been released:
https://github.com/postcss/postcss-selector-parser/releases/tag/v6.0.16

Copy link
Contributor

@Mouvedia Mouvedia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the changes I have requested are non-blocking.
Feel free to integrate them at your convenience.

LGTM

@ybiquitous
Copy link
Member

Thank you for creating and reviewing the pull request!

@ybiquitous ybiquitous merged commit 5078666 into stylelint:main Mar 13, 2024
16 checks passed
@FloEdelmann FloEdelmann deleted the ignore-max-compound-selectors branch March 14, 2024 06:32
@FloEdelmann
Copy link
Member Author

Thanks for bringing this over the finish line!

@Mouvedia
Copy link
Contributor

Mouvedia commented Mar 14, 2024

@ybiquitous we may have missed something apparently.
relevant ref: #7544 (comment)

for config: [2, { ignoreSelectors: ['.ignored'] }],
and '.foo .bar > .ignored.qux {}'
what would you expect to happen?

I would expect the compound selector to not be ignored and hence end up with 3 and the test to fail instead of passing currently. Am i wrong?
Should we replace the current example to illustrate the unexpected behaviour?
e.g. change the misleading

.foo .bar > .ignored.ignored {}

to .foo .bar > .ignored.qux {}


Also even more baffling .foo .bar > .qux.ignored {} fails but .foo .bar > .ignored.qux {} doesn't. That's seems inconsistent to me.
i.e. either both fails or both passes ('.ignored' doesn't mean startsWith)

@ybiquitous
Copy link
Member

@Mouvedia Thanks, your concern is right. We have to fix the case.

The following makes sense to me, so I'll open a follow-up PR soon:

.foo .bar > .ignored.qux {}
.foo .bar > .qux.ignored {}
/* considered as ↓ */
.foo .bar > .qux {}

ybiquitous added a commit that referenced this pull request Mar 14, 2024
…s selectors

Follow-up to PR #7544
See #7544 (comment)

Note that this change doesn't need any changelog item
since the `ignoreSelectors` option is unreleased.
@ybiquitous
Copy link
Member

Opened follow-up PR #7559

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

Successfully merging this pull request may close these issues.

Add ignoreSelectors: [] to selector-max-compound-selectors
3 participants