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

Only report immutable class selectors, add ignoreSelectors option #43

Merged
merged 12 commits into from Sep 20, 2019

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Sep 18, 2019

After testing this in github/github, I noticed that there were a bunch of false positives for non-class selectors like ul, li, etc. This is an update to the work in #37 that:

  1. Changes the selector testing logic to report if the entire selector is itself marked as "immutable" (a duplicate of any Primer CSS selector) and it's a class selector (with or without pseudo-class).
  2. Returns on the first violation reported.
  3. Reduces the noise in error messages if the entire selector is a class selector (rather than repeating the "in <selector>" bit at the end).
  4. Adds a new ignoreSelectors rule option that allows you to ignore selectors containing one or more strings, matching one or more regular expressions, or satisfying a function.
  5. Improves the legibility of the error messages (a lot, IMO).

@shawnbot shawnbot mentioned this pull request Sep 18, 2019
2 tasks
const config = {
plugins: [require.resolve('../plugins/no-override')],
rules: {
'primer/no-override': [true, {bundles: ['base']}]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test tells the primer/no-override rule only to match selectors in the "base" bundle, which is where we normalize element styles, including hr.

__tests__/no-override.js Outdated Show resolved Hide resolved
@@ -4,6 +4,7 @@ const {requirePrimerFile} = require('../src/primer')
const ruleName = 'primer/no-override'
const CLASS_PATTERN = /(\.[-\w]+)/
const CLASS_PATTERN_ALL = new RegExp(CLASS_PATTERN, 'g')
const CLASS_PATTERN_ONLY = /^\.[-\w]+(:{1,2}[-\w]+)?$/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aaaand here's the ugly part: a regular expression that matches any class selector with an optional pseudo-class.

@shawnbot shawnbot changed the title Only report immutable *class* selectors Only report immutable class selectors, add ignoreSelectors option Sep 18, 2019
@shawnbot shawnbot mentioned this pull request Sep 19, 2019
2 tasks
@shawnbot shawnbot removed the request for review from colebemis September 19, 2019 18:45
@shawnbot shawnbot changed the base branch from release-8.0.1 to release-8.1.0 September 19, 2019 18:46
@shawnbot shawnbot merged commit 877594c into release-8.1.0 Sep 20, 2019
@shawnbot shawnbot deleted the no-override-classes branch September 20, 2019 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant