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.

Sign up for GitHub

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

Jump to bottom

Add checkContextFunctionalPseudoClasses: [] to selector-max-id #7380

Merged
merged 27 commits into from Jan 9, 2024
Merged

Add checkContextFunctionalPseudoClasses: [] to selector-max-id #7380

merged 27 commits into from Jan 9, 2024

Conversation

brigitamaria
Copy link
Contributor

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

Proof of concept for #7364

Is there anything in the PR that needs further explanation?

Yes, the documentation needs to be updated and I still have a type error that I'd like to discuss

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 8, 2023 •
edited

🦋 Changeset detected

Latest commit: 9b6b8d9

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

@brigitamaria brigitamaria marked this pull request as draft December 8, 2023 15:50
@brigitamaria brigitamaria marked this pull request as ready for review December 12, 2023 09:36
@brigitamaria brigitamaria marked this pull request as draft December 12, 2023 09:42
jeddy3
jeddy3 reviewed Dec 12, 2023
View reviewed changes
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@brigitamaria Thanks for the PR! The draft is looking good.

lib/rules/selector-max-id/__tests__/index.mjs Outdated
config: [
0,
{
includeContextFunctionalPseudoClasses: [':global'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
includeContextFunctionalPseudoClasses: [':global'],
includeContextFunctionalPseudoClasses: [':--foo'],

Let's use custom pseudos in the test and documentation to avoid using non-standard syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated f310fa7 😄 thanks for your feedback!

@brigitamaria brigitamaria marked this pull request as ready for review December 13, 2023 10:33
@jeddy3 jeddy3 changed the title Additional param: includeContextFunctionalPseudoClasses for selector-max-id Add includeContextFunctionalPseudoClasses: [] to selector-max-id Dec 16, 2023
jeddy3
jeddy3 requested changes Dec 16, 2023
View reviewed changes
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@brigitamaria Thank you for the PR.

I've requested some changes to the tests and docs to align with our conventions.

lib/rules/selector-max-id/README.md Outdated Show resolved Hide resolved
lib/rules/selector-max-id/README.md Outdated Show resolved Hide resolved
lib/rules/selector-max-id/README.md Outdated Show resolved Hide resolved
lib/rules/selector-max-id/README.md Outdated Show resolved Hide resolved
lib/rules/selector-max-id/README.md Outdated Show resolved Hide resolved
lib/rules/selector-max-id/__tests__/index.mjs Outdated Show resolved Hide resolved
lib/rules/selector-max-id/__tests__/index.mjs Outdated Show resolved Hide resolved
lib/rules/selector-max-id/__tests__/index.mjs Outdated Show resolved Hide resolved
lib/rules/selector-max-id/__tests__/index.mjs Outdated Show resolved Hide resolved
lib/rules/selector-max-id/__tests__/index.mjs Outdated Show resolved Hide resolved
@jeddy3
Update lib/rules/selector-max-id/README.md
00a5785
@jeddy3
Update lib/rules/selector-max-id/README.md
d8db884
@jeddy3
Update lib/rules/selector-max-id/README.md
52e5d65
@jeddy3
Update lib/rules/selector-max-id/README.md
dabed8f
@jeddy3
Update lib/rules/selector-max-id/README.md
812272d
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
df3006f
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
9fae7e4
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
28ebd68
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
448d958
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
a8e6968
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
46eec07
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
d2b4c28
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
c10237b
@jeddy3
Update lib/rules/selector-max-id/__tests__/index.mjs
9d7ff41
@jeddy3
Update README.md
ff43062
@jeddy3 jeddy3 changed the title Add includeContextFunctionalPseudoClasses: [] to selector-max-id Add checkContextFunctionalPseudoClasses: [] to selector-max-id Jan 7, 2024
@jeddy3
Copy link
Member

jeddy3 commented Jan 7, 2024

On revisiting this issue, I realised we should call the option checkContextFunctionalPseudoClasses as we have used check* before when making a rule stricter.

I've committed my suggestions and renamed the option.

PR LGTM, but it could do with a second pair of eyes.

@jeddy3 jeddy3 mentioned this pull request Jan 8, 2024
Closed
4 tasks
@ybiquitous ybiquitous linked an issue Jan 9, 2024 that may be closed by this pull request
Add checkContextFunctionalPseudoClasses: [] to selector-max-id #7364
Closed
ybiquitous
ybiquitous reviewed Jan 9, 2024
View reviewed changes
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] Thank you for the pull request. LGTM too.

In addition, I suggest the refactoring to make the complex if condition a bit more readable:

  • Move the isContextFunctionalPseudoClass call into isIgnoredContextFunctionalPseudoClass
  • Rename isIgnoredContextFunctionalPseudoClass to isUnignoredContextFunctionalPseudoClass
  • Check a node's type in isCheckedContextFunctionalPseudoClasses
  • Rename isCheckedContextFunctionalPseudoClasses to isCheckedContextFunctionalPseudoClass

lib/rules/selector-max-id/index.mjs Outdated Show resolved Hide resolved
lib/rules/selector-max-id/index.mjs Show resolved Hide resolved
@jeddy3
Copy link
Member

jeddy3 commented Jan 9, 2024

@ybiquitous Great suggestions. I've made the changes.

ybiquitous
ybiquitous approved these changes Jan 9, 2024
View reviewed changes
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.

LGTM!

@jeddy3 jeddy3 merged commit 5f5cada into stylelint:main Jan 9, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ybiquitous ybiquitous ybiquitous approved these changes

@jeddy3 jeddy3 jeddy3 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add checkContextFunctionalPseudoClasses: [] to selector-max-id
3 participants
@brigitamaria @jeddy3 @ybiquitous