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 rule-selector-property-disallowed-list rule #5489

Conversation

doing-art
Copy link
Contributor

@doing-art doing-art commented Aug 22, 2021

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

The new rule rule-selector-property-disallowed-list that is mentioned in #5433 has been added.

Is there anything in the PR that needs further explanation?

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

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.

Thank you for creating the PR! 👏🏼

lib/rules/rule-selector-property-disallowed-list/index.js Outdated Show resolved Hide resolved
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! 👏🏼

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.

Thanks for continuing to work on this pull request.

I've requested some changes to:

  • align with our conventions
  • test multi-value behaviour

lib/rules/rule-selector-property-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/rule-selector-property-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/rule-selector-property-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/rule-selector-property-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/rule-selector-property-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/rule-selector-property-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/rule-selector-property-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/rule-selector-property-disallowed-list/README.md Outdated Show resolved Hide resolved
line: 1,
column: 19,
},
],
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a reject test for two violations, e.g.

code: 'a { color: red; margin-top: 0px; }',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeddy3 Thank you for your review. Updated the tests as you suggested.

@jeddy3 jeddy3 changed the title Add new rule-selector-property-disallowed-list rule Add rule-selector-property-disallowed-list rule Oct 11, 2021
doing-art and others added 13 commits October 15, 2021 14:18
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
…ex.js

Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@doing-art doing-art requested a review from jeddy3 October 15, 2021 12:07
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.

Thanks for making the changes. It's looking good.

I've one last request. Let's add a reject test for:

code: 'a { color: red; margin-top: 0px; }',

It should report two problems. We'll need to use the warnings property. Something like:

{
	code: "a { color: red; margin-top: 0px; }"
	warnings: [
		{ message: messages.rejected('color', 'a'), line: 1, column: 5 },
		{ message: messages.rejected('margin-top', 'a'), line: 1, column: 17 },
	],
}

@jeddy3 jeddy3 deleted the branch stylelint:v14 October 21, 2021 13:36
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.

None yet

3 participants