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

Fixed: selector-type-no-unknown is now ignore /deep/ shadow-piercing descendant combinator. #2508

Merged
merged 1 commit into from
Apr 22, 2017
Merged

Fixed: selector-type-no-unknown is now ignore /deep/ shadow-piercing descendant combinator. #2508

merged 1 commit into from
Apr 22, 2017

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Apr 18, 2017 •
edited
Loading

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

#2506

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

Maybe need update docs ❓ or just add example to valid ❓

jeddy3
jeddy3 requested changes Apr 18, 2017
View reviewed changes
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.

Tests look good.

I've made a request to use a util.

lib/rules/selector-type-no-unknown/index.js Outdated
@@ -89,6 +89,7 @@ const rule = function (actual, options) {
|| svgTags.indexOf(tagNameLowerCase) !== -1
|| keywordSets.nonStandardHtmlTags.has(tagNameLowerCase)
|| mathMLTags.indexOf(tagNameLowerCase) !== -1
|| tagNameLowerCase === "/deep/"
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 move this to the isStandardSyntaxTypeSelector util. It's where we build on top of postcss-selector-parser.

Also, let's return false for all reference combinators, not just /deep/ i.e. return false for anything that starts and ends with a /.

Be sure to comment the line e.g.

// Reference combinators like `/deep/`

Copy link
Member Author

Choose a reason for hiding this comment

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

@jeddy3 good solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jeddy3 one remarks: remove tests on invalid /deeeps/ (i think it is not related to this rules).

@alexander-akait alexander-akait force-pushed the issue-2506-selector-type-no-unknown branch from 4a40256 to 30311a7 Compare April 18, 2017 11:52
@alexander-akait
Copy link
Member Author

/cc @jeddy3

jeddy3
jeddy3 approved these changes Apr 18, 2017
View reviewed changes
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.

LGTM

davidtheclark
davidtheclark approved these changes Apr 21, 2017
View reviewed changes
Copy link
Contributor

@davidtheclark davidtheclark left a comment

Choose a reason for hiding this comment

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

One question, otherwise seems fine to me.

lib/rules/selector-type-no-unknown/__tests__/index.js
description: "shadow-piercing descendant combinator",
}, {
code: ".foo /for/ .bar {}",
description: "reference combinators",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a "reference combinator"?

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark https://www.w3.org/TR/selectors4/#idref-combinators

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark but seem it is deprecated selector, i don't see infomation about this in other spec, btw some project use /deep/ and be good just ignore this

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@davidtheclark davidtheclark merged commit 311e66e into master Apr 22, 2017
@davidtheclark davidtheclark deleted the issue-2506-selector-type-no-unknown branch April 22, 2017 17:57
@davidtheclark
Copy link
Contributor

Added to changelog:

  • Fixed: selector-type-no-unknown now ignores the /deep/ shadow-piercing combinator (#2508).

@davidtheclark davidtheclark mentioned this pull request Apr 22, 2017
Closed
@renovate renovate bot mentioned this pull request Jun 9, 2017
Merged
@dependabot-preview dependabot-preview bot mentioned this pull request Mar 14, 2018
Closed
@EmilyRosina
Copy link

I still don't understand how to implement this rule in my project 😭 but i need to as we always use ::v-deep now - HELP 🙏

@jeddy3
Copy link
Member

jeddy3 commented Sep 16, 2019

@EmilyRosina I don't believe this PR is related to the ::v-deep pseudo-element.

Please create a new issue, following the bug template, if you're getting an unexpected violation.

@EmilyRosina
Copy link

EmilyRosina commented Sep 16, 2019 •
edited
Loading

@jeddy3 I just wanted a way to tell the linter that we want to see an error if /deep/ or >>> are used.
I assume this does not need a new issue?

@jeddy3
Copy link
Member

jeddy3 commented Sep 16, 2019 •
edited
Loading

You can use the selector-combinator-blacklist rule to disallow the >>> combinator:

{
  "rules": {
    "selector-combinator-blacklist": [">>>"],
  }
}

The /deep/ reference combinator is more problematic as the selector-combinator-blacklist rule ignores reference combinators. However, you can write a plugin to target and disallow these unusual constructs. I suggest you call the plugin stylelint-selector-reference-combinator-blacklist. You can use the code, tests and documentation of the built-in selector-combinator-blacklist rule as a blueprint.

@EmilyRosina
Copy link

Ah ok, so there is no way to do this straight up with stylelint atm 😞
I guess we'll just rely on Vue/Chrome (unusre who to blame/thank) to tell us for now if someone used /deep/. Just a shame we cannot easily do it through the linter.

Thanks anyway 👍

@jeddy3
Copy link
Member

jeddy3 commented Sep 16, 2019

Just a shame we cannot easily do it through the linter.

You can easily do it by writing a plugin. The plugin system exists for exactly these types of scenarios.

@EmilyRosina
Copy link

It looked a little in-depth on first glance, but maybe it's not as bad as i think 😅
I will have a look once i have more time 😸

@ybiquitous ybiquitous mentioned this pull request Oct 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtheclark davidtheclark davidtheclark approved these changes

@jeddy3 jeddy3 jeddy3 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alexander-akait @davidtheclark @EmilyRosina @jeddy3