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 media-feature-name-value-whitelist #3312

Closed
MatthiasKunnen opened this issue May 15, 2018 · 12 comments
Closed

Add media-feature-name-value-whitelist #3312

MatthiasKunnen opened this issue May 15, 2018 · 12 comments
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone type: new rule an entirely new rule

Comments

@MatthiasKunnen
Copy link
Member

MatthiasKunnen commented May 15, 2018

Is your feature request related to a problem? Please describe.
People sometimes use breakpoints at the weirdest values. This adds unnecessary complexity to understanding the points on which the UI changes.

Describe the solution you'd like
I would like to propose a breakpoint-strict rule which takes a list of values that can be used in media queries. There would be two lists that can be passed, one for min-width and one for max-width. This to avoid issues with overlapping values as explained in #3311.

There could be a possibility to add presets such as the Material breakpoints and Bootstrap.

@alexander-akait
Copy link
Member

@MatthiasKunnen Please use issue template

@MatthiasKunnen
Copy link
Member Author

Is this not conform to the issue template? What can I do to make it so?

First paragraph is describing the problem, second one is the solution I would like to see in order to mitigate aforementioned problem. There are no alternatives I've considered apart from detecting this in PR reviews but I figured I would leave that out of this feature request as it is the way all linting can be done.

I'm willing to learn how to make this rule but I thought it'd be best to mention my proposal in order to get some feedback so that when I create it, it can help other people.

@alexander-akait
Copy link
Member

alexander-akait commented May 15, 2018

@MatthiasKunnen

🚀 Feature request
Suggest a new feature, new rule, or new option idea for stylelint

https://github.com/stylelint/stylelint/issues/new?template=Feature_request.md

Looks on your description your request is new rule, right?

/cc @ntwb what is wrong with our templates? People do not want to use it

@MatthiasKunnen
Copy link
Member Author

@evilebottnawi Yes, a new rule indeed.
I added the titles from the template, is this OK? I thought the template was a desired structure of the feature request and I removed the titles after filling them in.

@hudochenkov
Copy link
Member

@MatthiasKunnen Thank you for using a template. These titles help us navigate in issue's description and outline its structure.

@MatthiasKunnen
Copy link
Member Author

@evilebottnawi @ntwb @hudochenkov Personally, I think I would be less inclined to delete the titles if they were less verbose. They look more like structure guidelines right now.

Current template:

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Proposed template

The problem
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Desired solution
A clear and concise description of what you want to happen.

Considered alternatives
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

@jeddy3
Copy link
Member

jeddy3 commented May 16, 2018

@MatthiasKunnen Thanks for the request. Also, thank you for the template feedback. It's much appreciated as the templates are new. I've created a separate issue to follow up on that: #3313.

This is another interesting proposal. I believe this one both meets the criteria for inclusion and is in line with the VISION.

I believe you're asking to apply a limit on the values of media features based on their names (ref). The prior art for something similar is the declaration-property-value-whitelist rule.

With that in mind, would the following satisfy your use case?:

  • Name: media-feature-name-value-whitelist
  • Description: Specify a whitelist of allowed name and value pairs within media features.
  • Primary option: object { string/regex: [string/regex] }
  • Secondary options: none
  • Message: "Unexpected value "${value}" for name "${name}""
  • Section: "Limit language features -> Media features"

For example:

{
  "rules": {
    "media-feature-name-value-whitelist": {
      "min-width": ["30rem", "48rem", "50rem"],
      "max-width": ["25rem"],
      "pointer": ["fine", "none"],
      "orientation": ["portrait"]
    }
  }
}

I believe the rule should ignore media features in either a boolean or range context.

I think this rule would make a good companion to the existing media-feature-name-whitelist one.

I also don't think there's a need for a corresponding media-feature-name-value-blacklist rule. We can add it when there's a real-world request for it.

There could be a possibility to add presets such as the Material breakpoints and Bootstrap.

A long-standing design decision is not to provide presets as options. In stylelint, these types of use cases are best covered as shared configs.

I'm willing to learn how to make this rule

That's fantastic! Fortunately, there is that rule to use as a blueprint. Also, there's a media query parser that would come in very handy. It will allow you to parse out the name and values of each media feature. There's lots of guidance on getting started in the developer manual. It'd be a great rule to work on and I agree that it would, among many uses, help remove unnecessary complexity to understanding the points on which the UI changes.

@jeddy3 jeddy3 changed the title breakpoint-strict proposal Add media-feature-name-value-whitelist May 16, 2018
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: new rule an entirely new rule good first issue is good for newcomers labels May 16, 2018
@MatthiasKunnen
Copy link
Member Author

@jeddy3 I agree with the name change and thank you for recommending how to get started.

Concerning boolean and range context media features, I also think they should not be subject to this rule. Maybe the range context media features can be subjected once their support becomes dominant and there is a demand.

I would change the message as follows:

- Unexpected value "${value}" for name "${name}"
+ Disallowed value "${value}" for "${name}"

Could we provide an option that allows any value to be used when the media feature is not specified? E.g.

lenient

{
    "rules": {
        "media-feature-name-value-whitelist": [
            "lenient",
            {
                "max-width": ["25rem"]
            }
        ]
    }
}

Allowed

@media all and (max-width: 25rem) {
}
@media all and (min-width: 320px) {
}

Violation

@media all and (max-width: 29rem) {
}

strict

{
    "rules": {
        "media-feature-name-value-whitelist": [
            "strict",
            {
                "max-width": ["25rem"]
            }
        ]
    }
}

Allowed

@media all and (max-width: 25rem) {
}

Violation

@media all and (max-width: 29rem) {
}
@media all and (min-width: 320px) {
}

@jeddy3
Copy link
Member

jeddy3 commented May 17, 2018

I would change the message as follows:

Thanks for the suggestion. There are lots of rules in stylelint and the rules messages follow conventions to keep them consistent. Unexpected value "${value}" for name "${name}" is in line with those conventions.

Could we provide an option that allows any value to be used when the media feature is not specified?

I don't believe those strict and lenient options would be consistent with the behaviour of the other *-whitelist rules. A user can also build on the built-in rules to extend or constrain them within their own plugin. So, I don't think we should, at this time, accommodate this use case in the rule itself.

@MatthiasKunnen
Copy link
Member Author

@jeddy3, I'm making progress on the rule, one thing I'm not sure of is how to handle non-standard syntax. On the one hand I don't want to see:

{
  "min-width": ["500px"]
}
$illegal-value: 777px;
@media screen and (min-width: $illegal-value) {
}

on the other hand, people will be using @media ($media-feature: $value) {} legitimately in mixins and functions. In that case, an error wouldn't be warranted. Any advice?

My assumption is that this case can only be filtered out during PRs.

@hudochenkov
Copy link
Member

I think it should be ignored.

User could specify list of allowed values which could include variables they use. E. g.:

{
  "min-width": ["500px", "$desktop-width", "$handheld-width"]
}

For your example $illegal-value is not in the list, so rule will report this violation.

@hudochenkov
Copy link
Member

We ignore non-standard syntaxes by default in our rules. If supporting non-standard syntax is easy to add and it's not ambiguous, then we can support non-standard syntax in the rule. But requirement is support standard CSS, all other syntaxes are optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone type: new rule an entirely new rule
Development

No branches or pull requests

4 participants