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

Added: `selector-max-type` rule #2665

Merged
merged 2 commits into from Jun 26, 2017

Conversation

3 participants
@evilebottnawi
Member

evilebottnawi commented Jun 21, 2017

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

#2528

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

Need resolve:

  1. html { --foo: 1px; } and html { --custom-property-set: {} } should be rejected? I think yes, because we can have html { --some-here: 1px; --custom-property-set: { color: red; display: block; }; color: red;} Later we can add option ignoreContaintCustom (or best name for this purpose).
  2. After resolve selector some non standard (example scss: @for $n from 1 through 5 { .foo-#{$n} { div { content: \"#{$n}\"; } } }) output error .foo-#{$n} div, it is valid because we contain div, but message is very weird. It is normal or not?

Some tests are commented.

@evilebottnawi evilebottnawi requested review from hudochenkov and jeddy3 Jun 21, 2017

@evilebottnawi evilebottnawi force-pushed the selector-max-type branch 2 times, most recently from 1f5a128 to 3110ec4 Jun 21, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 21, 2017

html { --foo: 1px; } and html { --custom-property-set: {} } should be rejected?

Yes, absolutely. Rules should be most restrictive by default (when it comes to standard CSS), and then made more permissive with options. Let's only add a ignoreContaintCustom option if it is requested. I suspect using the existing ignoreTypes: ["html"] option will suffice for most users.

.foo-#{$n} div

Shouldn't this whole selector be ignored by the rule as it's non-standard? We should check that resolved selectors are standard. If not, return early.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 21, 2017

Non-standard selectors should be ignored, and tests for them should be accepted.

@jeddy3

jeddy3 approved these changes Jun 26, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 26, 2017

@hudochenkov Updated this one.

  • Added a few extra reject tests to be sure the ignore options are working as expected.
  • Fixed test descriptions
  • Updated rule description
  • Added pattern examples

@jeddy3 jeddy3 changed the title from WIP: Added: `selector-max-type` rule to Added: `selector-max-type` rule Jun 26, 2017

@jeddy3 jeddy3 referenced this pull request Jun 26, 2017

Closed

Release 7.12 #2574

5 of 5 tasks complete
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 26, 2017

@hudochenkov Once you've approved my commits. I'll squash them and rebase this off master.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jun 26, 2017

Why we don't ignore non-standard selectors?

.foo-#{$n} div

Shouldn't this whole selector be ignored by the rule as it's non-standard? We should check that resolved selectors are standard. If not, return early.

Non-standard selectors should be ignored, and tests for them should be accepted.

@hudochenkov Oops. I missed that one. Changed.

@hudochenkov

Good work!

@jeddy3 jeddy3 force-pushed the selector-max-type branch from 2836bd1 Jun 26, 2017

@jeddy3 jeddy3 force-pushed the selector-max-type branch to 184043d Jun 26, 2017

@jeddy3 jeddy3 merged commit bf8e283 into master Jun 26, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 95.673%
Details

@jeddy3 jeddy3 deleted the selector-max-type branch Jun 26, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jun 26, 2017

  • Added: selector-max-type rule (#2665).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment