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 ignoreMediaFeatureNames for unit-blacklist #3027

Merged
merged 3 commits into from Nov 23, 2017

Conversation

3 participants
@lucmerceron
Contributor

lucmerceron commented Nov 18, 2017

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

#3006

Is there anything in the PR that needs further explanation?

The PR is easy to understand.

NB:
I am not sure this solution is the most optimized since I have one loop for the value valueParser inside the one for the media mediaParser but it feels safer than doing matches only on the returned mediaFeatureNode and having to add column size for error handling.

Since this is my first PR for stylelint, this is widely open to discussion. I may have missed a util or anything else that could be used.

}
}
);
if (!validOptions) {
return;
}
function check(node, value, getIndex) {
function checkMedia(node, value, getIndex) {
value = value.replace(/\*/g, ",");

This comment has been minimized.

@CAYdenberg

CAYdenberg Nov 18, 2017

Contributor

Let's leave this line out. Its sole purpose is handle parsing of calc functions - I don't think it's needed within media queries (and it's a bit of a hack so I'd like to not proliferate it if possible).

This comment has been minimized.

@lucmerceron

lucmerceron Nov 19, 2017

Contributor

Done in my last commit b6effdf

return;
}
const unit = getUnitFromValueNode(valueNode);

This comment has been minimized.

@CAYdenberg

CAYdenberg Nov 18, 2017

Contributor

From here to the end of the function is almost a mirror image of the same thing in walk.decls. I think it should be abstracted.

This comment has been minimized.

@lucmerceron

lucmerceron Nov 19, 2017

Contributor

Done in my last commit too b6effdf

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Nov 18, 2017

Thanks. Tests and documentation looks good.

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Nov 19, 2017

Awesome. Thanks again.

@jeddy3

@lucmerceron Many thanks. The code and tests look great to me!

I've made a couple of very minor documentation nit-picks. There is a lot of documentation in stylelint, so we try to keep things as consistent as possible for the benefit of the user.

The following patterns are considered violations:
```css
a { @media screen and (max-device-width: 500px) { } }

This comment has been minimized.

@jeddy3

jeddy3 Nov 22, 2017

Member

Can we just have @media screen and (max-device-width: 500px) {} here please as I don't believe nested media queries are standard syntax.

This comment has been minimized.

@lucmerceron

lucmerceron Nov 23, 2017

Contributor

Done in my last commit

The following patterns are *not* considered violations:
```css
@media (min-width: 960px) { }

This comment has been minimized.

@jeddy3

jeddy3 Nov 22, 2017

Member

Can you please change the examples to use empty braces i.e. {} as per the guidelines.

This comment has been minimized.

@lucmerceron

lucmerceron Nov 23, 2017

Contributor

Yep, sorry about that. Done in my last commit.

@jeddy3

jeddy3 approved these changes Nov 23, 2017

@lucmerceron Thanks for the quick changes. LGTM.

@CAYdenberg CAYdenberg merged commit ec2a52e into stylelint:master Nov 23, 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.683%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment