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

Fix selector-pseudo-class-no-unknown false positive for :modal #6811

Merged
merged 3 commits into from May 1, 2023
Merged

Fix selector-pseudo-class-no-unknown false positive for :modal #6811

merged 3 commits into from May 1, 2023

Conversation

Yasir761
Copy link
Contributor

@Yasir761 Yasir761 commented Apr 24, 2023

Closes #6809.

Is there anything in the PR that needs further explanation?

e.g. "No, it's self-explanatory."

@changeset-bot
Copy link

changeset-bot bot commented Apr 24, 2023

🦋 Changeset detected

Latest commit: 3e601d8

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

Copy link
Member

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

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

Hi @Yasir761, thanks for submitting a change! Two quick asks:

  1. could you add a test case that validates your fix?
  2. could you add a changeset using npx changeset?

(I've renamed the PR to match our stylistic rules; you can use the title for the changeset; just change the tense)

@mattxwang mattxwang changed the title Fix pseudo class Fix selector-pseudo-class-no-unknown false positive for :modal Apr 24, 2023
@Yasir761
Copy link
Contributor Author

Hi @Yasir761, thanks for submitting a change! Two quick asks:

  1. could you add a test case that validates your fix?
  2. could you add a changeset using npx changeset?

(I've renamed the PR to match our stylistic rules; you can use the title for the changeset; just change the tense)

off course I'll add these, can you tell me where I should add test case?

@mattxwang
Copy link
Member

off course I'll add these, can you tell me where I should add test case?

@Yasir761 each rule has a __tests__ subdirectory; so, I'd add it within lib/rules/selector-pseudo-class-no-unknown/__tests__/index.js. Since this is a false positive, you'll want to write a simple accept case that uses :modal. Try to match the conventions we use for other test cases; for more, see the writing tests section of our developer guide!

Copy link
Member

@mattxwang mattxwang 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 submitting some changes @Yasir761; I've added some comments, could you take a look at them and let me know what you think!

(as a suggestion: make sure that npm test passes locally!)

"stylelint": minor
---

Fixed selector-pseudo-class-no-unknown false positive for :modal
Copy link
Member

Choose a reason for hiding this comment

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

Changeset nit:

Suggested change
Fixed selector-pseudo-class-no-unknown false positive for :modal
Fixed: `selector-pseudo-class-no-unknown` false positive for `:modal`

Comment on lines 133 to 140
{
code: 'a:modal { }',
message: messages.rejected(':modal'),
line: 1,
column: 2,
endLine: 1,
endColumn: 10,
},
Copy link
Member

Choose a reason for hiding this comment

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

Hm, it's unclear to me why this test case is here; we don't want to reject :modal since it is a known pseudo-class. Could you remove this test case?

Suggested change
{
code: 'a:modal { }',
message: messages.rejected(':modal'),
line: 1,
column: 2,
endLine: 1,
endColumn: 10,
},

@@ -28,6 +28,9 @@ testRule({
{
code: 'a::before { }',
},
{
code: 'a:modal { }',
Copy link
Member

Choose a reason for hiding this comment

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

Bit of a nit, but it seems like from documentation (MDN, CSS spec) that no attached selector is more idiomatic:

Suggested change
code: 'a:modal { }',
code: ':modal { }',

Copy link
Member

@mattxwang mattxwang left a comment

Choose a reason for hiding this comment

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

Thanks - LGTM!

@mattxwang mattxwang merged commit e805310 into stylelint:main May 1, 2023
14 checks passed
@Yasir761 Yasir761 deleted the fix-pseudo-class branch May 2, 2023 09:43
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.

Fix selector-pseudo-class-no-unknown false positive for :modal
2 participants