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 selector-pseudo-element-*list rules #3087

Merged
merged 5 commits into from
Jan 6, 2018
Merged

Add selector-pseudo-element-*list rules #3087

merged 5 commits into from
Jan 6, 2018

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Dec 29, 2017

Closes #3014

It’s been a while since I had the time to write some actual stylelint code, especially adding new rules.

I found myself reaching for the selector-pseudo-element-whitelist rule on my current project, so figured I’d add it.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Good job, @jeddy3!

It’s been a while since I had the time to write some actual stylelint code, especially adding new rules.

Feels good, right? :)

Both rules checking CSS3 pseudo-elements only. CSS2 also has :before and :after pseudo-elements. I see two options: support these selectors, or add a note about this limitation to readme.

return (root, result) => {
const validOptions = validateOptions(result, ruleName, {
actual: blacklist,
possible: [_.isString]
Copy link
Member

Choose a reason for hiding this comment

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

It should also check for _.isRegExp according to readme. It would be great to have RegExp in tests as well. Both as a string and as RegExp.

The same comment applies to the second rule.

@jeddy3
Copy link
Member Author

jeddy3 commented Dec 30, 2017

@hudochenkov Thanks for the feedback. I've made the changes.

I'm about to head out, so I've pushed up what I've got... and there's a flow error. Any flow pros know how to fix this? Setting the type to string | Class<RegExp> gives:

^^^^ property `test`. Property not found in statics of `RegExp`

@CAYdenberg
Copy link
Contributor

Right, so flow doesn't understand that _.isRegExp is ensuring this is a regular expression. It thinks comparison could be a RegExp or a string, so it's warning you that .test doesn't work for strings.

Try using typeof comparison (harder for the reader, since typeof /myregexp/ === "object") or

if (comparison.test) {
    // now we know this is RegExp and we can CALL comparison.test
}

Make sense?

@jeddy3
Copy link
Member Author

jeddy3 commented Jan 1, 2018

Make sense?

Yes, thanks very much @CAYdenberg


This PR is ready for review again.

@jeddy3
Copy link
Member Author

jeddy3 commented Jan 1, 2018

@hudochenkov Restarted travis and now CI is green.

@ntwb ntwb mentioned this pull request Jan 3, 2018
6 tasks
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Good job!

@jeddy3
Copy link
Member Author

jeddy3 commented Jan 4, 2018

@stylelint/core Ready for a 2nd review

Copy link
Member

@ntwb ntwb 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 4c90af5 into master Jan 6, 2018
@jeddy3 jeddy3 deleted the issue-3014 branch January 6, 2018 15:21
@jeddy3
Copy link
Member Author

jeddy3 commented Jan 6, 2018

  • Added: selector-pseudo-element-*list rules (#3104).

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

4 participants