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-range-notation #6497

Merged
merged 4 commits into from Dec 1, 2022
Merged

Add media-feature-range-notation #6497

merged 4 commits into from Dec 1, 2022

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Nov 29, 2022

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

Closes #6161

Is there anything in the PR that needs further explanation?

Part of cleaning up our pull requests as we prepare for v15.

As mentioned in #6430 (comment), this pull request doesn't include autofix. I'll open a new issue for that. CssTools dropped a new media query parser the other day that may make implementating autofix more viable.

Additionally, I believe the edge-case of:

@media (min-width: 500px) and screen and (max-width: 1200px) {}

Should be transformed by this rule into:

@media (width >= 500px) and screen and (width <= 1200px) {}

That could then be merged into:

@media screen and (500px <= width <= 1200px) {}

By another rule, one that sets the notation of media features in a range context. For example, media-feature-range-context-notation: "separate"|"combined". I'll create another issue to discuss this, though. I wanted to mention it here to give context to this pull request.

There are lots of terms in media queries. I've tried to get them right in the rule logic and test descriptions. Casting a second pair of eyes over these would be great.

@changeset-bot
Copy link

changeset-bot bot commented Nov 29, 2022

🦋 Changeset detected

Latest commit: 7d3c192

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Mouvedia
Copy link
Contributor

nit

add Co-authored-by: name <name@example.com> trailer for @sidverma32

Co-authored-by: sidverma32 <sid.verma32@gmail.com>
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thanks for creating the pull request. I've left some trivial suggestions, but there are no big problems. 👍🏼

lib/rules/media-feature-range-notation/README.md Outdated Show resolved Hide resolved
lib/rules/media-feature-range-notation/README.md Outdated Show resolved Hide resolved
lib/rules/media-feature-range-notation/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/media-feature-range-notation/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/media-feature-range-notation/__tests__/index.js Outdated Show resolved Hide resolved
jeddy3 and others added 2 commits November 30, 2022 10:15
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM 👍🏼

Let's add a changelog entry before merging the PR!

@jeddy3
Copy link
Member Author

jeddy3 commented Nov 30, 2022

I've left some trivial suggestions, but there are no big problems

All good catches, thanks.

I've added a changelog entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add media-feature-range-notation
3 participants