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 at-mixin-no-risky-nesting-selector rule. #985

Merged
merged 1 commit into from May 3, 2024
Merged

Add at-mixin-no-risky-nesting-selector rule. #985

merged 1 commit into from May 3, 2024

Conversation

pamelalozano16
Copy link
Contributor

Fix #892.

nex3
nex3 suggested changes Apr 2, 2024
View reviewed changes
src/rules/index.js Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixins-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
@pamelalozano16 pamelalozano16 changed the title Add mixins-no-risky-parent-selectors rule. Add mixin-no-risky-parent-selectors rule. Apr 2, 2024
@pamelalozano16 pamelalozano16 requested a review from nex3 April 2, 2024 22:38
@pamelalozano16 pamelalozano16 marked this pull request as ready for review April 3, 2024 17:51
nex3
nex3 suggested changes Apr 5, 2024
View reviewed changes
src/rules/mixin-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixin-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/mixin-no-risky-parent-selectors/__tests__/index.js Outdated Show resolved Hide resolved
src/rules/mixin-no-risky-parent-selectors/__tests__/index.js Outdated Show resolved Hide resolved
src/rules/mixin-no-risky-parent-selectors/index.js Outdated Show resolved Hide resolved
@pamelalozano16 pamelalozano16 force-pushed the risky-parent-selectors branch 2 times, most recently from b856462 to a834cb4 Compare April 8, 2024 18:33
@pamelalozano16 pamelalozano16 requested a review from nex3 April 8, 2024 18:34
@pamelalozano16 pamelalozano16 requested a review from nex3 April 8, 2024 23:03
@kristerkari
Copy link
Collaborator

Thanks! I'll need to review this, but after a quick look I noticed that the rule name starts with mixin-. There is already a category of rules with the prefix at-mixin-, and this rule should be put there. 👍

@pamelalozano16
Copy link
Contributor Author

@kristerkari Any thoughts on this? :)

@kristerkari
Copy link
Collaborator

@pamelalozano16 I had a quick glance, at it looks good, but I would still like to properly read through the changes. I should have some time tomorrow to sit down and do some PR reviewing 👍

@kristerkari
Copy link
Collaborator

@pamelalozano16 also, did you notice my comment about the rule name (mixin-no-risky-parent-selectors -> at-mixin-no-risky-parent-selectors)?

@pamelalozano16 pamelalozano16 force-pushed the risky-parent-selectors branch 2 times, most recently from 1ec3c7c to 903e7d0 Compare April 29, 2024 20:36
@pamelalozano16 pamelalozano16 changed the title Add mixin-no-risky-parent-selectors rule. Add at-mixin-no-risky-parent-selectors rule. Apr 29, 2024
kristerkari
kristerkari requested changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@kristerkari kristerkari left a comment

Choose a reason for hiding this comment

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

Still one comment about the rule name since I'm trying to keep the rule names consistent.

It seems like the most of the rules that start with the no- prefix are ending in a plural form (no-dollar-variables, no-duplicate-mixins, no-global-function-names, etc.).

The rules that start with something else seem to be mostly ending in a singular word. In this case it would mean that -selectors could be changed to selector.

Another thing is related to the naming of &. There is already a rule called selector-no-redundant-nesting-selector. I'm not sure what is the term that Sass uses, but it might be beneficial to keep it consistent unless using nesting-selector makes the name unclear.

This would mean that the name would be at-mixing-no-risky-nesting-selector. What do you think, would that name work?

src/rules/at-mixin-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
src/rules/at-mixin-no-risky-parent-selectors/__tests__/index.js Outdated Show resolved Hide resolved
src/rules/at-mixin-no-risky-parent-selectors/README.md Outdated Show resolved Hide resolved
@pamelalozano16
Copy link
Contributor Author

Yes! Did you mean at-mixin-no-risky-nesting-selector ? If yes, I agree that's fine.

@pamelalozano16 pamelalozano16 force-pushed the risky-parent-selectors branch 2 times, most recently from 9c00f4d to 5b83763 Compare May 2, 2024 22:15
@kristerkari
Copy link
Collaborator

Yes! Did you mean at-mixin-no-risky-nesting-selector ? If yes, I agree that's fine.

Yeah! Sorry, I had a typo in the name.

@kristerkari kristerkari changed the title Add at-mixin-no-risky-parent-selectors rule. Add at-mixin-no-risky-nesting-selector rule. May 3, 2024
kristerkari
kristerkari approved these changes May 3, 2024
View reviewed changes
Copy link
Collaborator

@kristerkari kristerkari left a comment

Choose a reason for hiding this comment

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

Looks good now 👍

@kristerkari
Copy link
Collaborator

A link to this rule missing in the repo README, but I can add that after merging.

@kristerkari kristerkari merged commit d06186d into stylelint-scss:master May 3, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nex3 nex3 nex3 approved these changes

@kristerkari kristerkari kristerkari approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a lint for dangerous parent selector use in mixins
3 participants
@pamelalozano16 @kristerkari @nex3