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

Add no-unknown-custom-media #7594

Merged
merged 9 commits into from
Apr 10, 2024
Merged

Add no-unknown-custom-media #7594

merged 9 commits into from
Apr 10, 2024

Conversation

fpetrakov
Copy link
Contributor

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

Closes #6362

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Apr 6, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 12d20ff

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

@fpetrakov fpetrakov force-pushed the rule/no-unknown-custom-media branch 3 times, most recently from daba0da to 2737a1d Compare April 7, 2024 16:37
@fpetrakov fpetrakov marked this pull request as ready for review April 7, 2024 16:38
@fpetrakov fpetrakov force-pushed the rule/no-unknown-custom-media branch from 3868e64 to 93a5543 Compare April 7, 2024 16:57
ybiquitous
ybiquitous requested changes Apr 8, 2024
View reviewed changes
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.

@fpetrakov Thank you for creating this pull request! The direction is correct, but I've suggested some bugfixes and refactorings. Can you take a look at them?

docs/user-guide/rules.md Outdated Show resolved Hide resolved
lib/rules/index.mjs Outdated Show resolved Hide resolved
lib/rules/no-unknown-custom-media/README.md Outdated Show resolved Hide resolved
lib/rules/no-unknown-custom-media/__tests__/index.mjs Outdated Show resolved Hide resolved
lib/rules/no-unknown-custom-media/__tests__/index.mjs Outdated Show resolved Hide resolved
lib/rules/no-unknown-custom-media/index.mjs Outdated Show resolved Hide resolved
lib/rules/no-unknown-custom-media/index.mjs Outdated Show resolved Hide resolved
lib/rules/no-unknown-custom-media/index.mjs Outdated Show resolved Hide resolved
lib/rules/no-unknown-custom-media/index.mjs Outdated Show resolved Hide resolved
types/stylelint/index.d.ts Outdated Show resolved Hide resolved
fpetrakov and others added 4 commits April 8, 2024 12:17
@fpetrakov @ybiquitous
Update docs/user-guide/rules.md
29633c8 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@fpetrakov @ybiquitous
Update lib/rules/index.mjs
01a5a45 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@fpetrakov @ybiquitous
Update lib/rules/no-unknown-custom-media/README.md
158af80 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@fpetrakov
fix things
b45e9cd
@fpetrakov
Copy link
Contributor Author

@ybiquitous thanks a lot for the review, learned a bunch of things

ybiquitous
ybiquitous approved these changes Apr 8, 2024
View reviewed changes
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 👍🏼

@Mouvedia
Copy link
Member

Mouvedia commented Apr 8, 2024 •
edited
Loading

There's something interesting about this rule addition.
AFAIK no browsers currently support @custom-media.
e.g. https://issues.chromium.org/issues/40781325
Which means the rule is being added on the premise that you are using a lowering tool.
Maybe we could link to those in the rule's README?
e.g.

@fpetrakov fpetrakov force-pushed the rule/no-unknown-custom-media branch from b15d52b to 2f9a6f7 Compare April 9, 2024 11:05
Mouvedia
Mouvedia approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@Mouvedia Mouvedia left a comment

Choose a reason for hiding this comment

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

LGTM

@ybiquitous
Copy link
Member

For #7594 (comment), I don't think adding some links to transpiling tools (e.g. postcss plugins or parcel) is necessary, even if there is insufficient support in the current browsers. We already have another rule for @custom-media, so we must keep such links up-to-date across documents.

See https://github.com/stylelint/stylelint/blob/08910de8bb3775b06efed7f7990e81d47548bace/lib/rules/custom-media-pattern/README.md

@ybiquitous ybiquitous merged commit dcb42d9 into stylelint:main Apr 10, 2024
16 checks passed
emmacharp pushed a commit to emmacharp/stylelint that referenced this pull request May 7, 2024
@fpetrakov @ybiquitous
@Mouvedia @emmacharp
Add no-unknown-custom-media (stylelint#7594)
bcc71e8 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Marc G <Mouvedia@users.noreply.github.com>
@OKEAMAH OKEAMAH mentioned this pull request May 26, 2024
Open
This was referenced Jun 3, 2024
Closed
Closed
Closed
Closed
@WontonSam WontonSam mentioned this pull request Jul 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mouvedia Mouvedia Mouvedia approved these changes

@ybiquitous ybiquitous ybiquitous approved these changes

@jeddy3 jeddy3 Awaiting requested review from jeddy3

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add no-unknown-custom-media
3 participants
@fpetrakov @Mouvedia @ybiquitous