Add media-feature-range-notation
#6430
Conversation
|
There was a problem hiding this comment.
@sidverma32 Thank you for making a start on this rule.
This rule will need to match the description in #6161 (comment) and #6161 (comment), i.e. it should allow our users to enforce either:
@media (min-width: 600px) {}or context notation:
@media (width >= 600px) {}for their media features.
It's probably best if you use one of the existing notation rules as a template. I suggest using the color-function-notation rule as the problem message is in the correct format. Then layer on the logic that traverses media queries from this pull request, and then add the logic to autofix media features in "prefix" notation into "context" notation.
From the description in #6161 (comment):
We'll only want to autofix the "context" primary option as it's the more modern notation that fixes shortcomings with the prefix notation. We can add autofixing later down the line if it's burdensome to add it upfront.
The rule will need to check all media features that define its “type” as “range” in its definition table in the spec. This includes the likes of
width,height,aspect-ratio,resolution,colorand so on.
We can look to postcss-media-minmax PostCSS plugin for inspiration on how we might implement these as our autofix for the "context" will work on the same constructs as the plugin but do the exact opposite.
As detailed in the developer guide, you'll also need to:
jeddy3
changed the title
media-feature-range-notation addedmedia-feature-range-notation
|
Looking at this some more, we may want to raise another issue for the autofix and tackle it in a separate pull request as there are some non-trival edge cases, e.g.: The following: @media (min-width: 500px) and screen and (max-width: 1200px) {}should be autofixed it: @media screen and (500px <= width <= 1200px) {}Let's focus on reporting problematic patterns in this pull request. We can rely on the media query parser's limitation of being unable to parse things in range context to keep the rule logic concise. Something along the lines of: const RANGE_TYPE_MEDIA_FEATURES = new Set([
"width",
"height",
"aspect-ratio",
"resolution",
"color",
]);
root.walkAtRules(/^media$/i, (atRule) => {
mediaParser(atRule.params).walk(/^media-feature$/i, (mediaFeatureNode) => {
const { parent, value } = mediaFeatureNode;
if (!isStandardSyntaxMediaFeature(value)) return;
if (
primary === "prefix" &&
(isPrefixedRangeTypeMediaFeature(value) ||
!isRangeTypeMediaFeature(value))
)
return;
if (
primary === "context" &&
(isRangeContextMediaFeature(value) || !isRangeTypeMediaFeature(value))
)
return;
const index = atRuleParamIndex(atRule) + parent.sourceIndex;
const endIndex = index + value.parent.length;
report({
message: messages.expected(primary),
node: atRule,
index,
endIndex,
result,
ruleName,
});
});
});
function isPrefixedRangeMediaFeature(mediaFeature) {
return mediaFeature.startsWith("min-") || mediaFeature.startsWith("max-");
}
function isRangeTypeMediaFeature(mediaFeature) {
const unprefixedMediaFeature = mediaFeature.replace(/^(min|max)-/, "");
return RANGE_TYPE_MEDIA_FEATURES.has(unprefixedMediaFeature);
}@sidverma32 The logic may prove too simplistic, but hopefully it gives you a sense of a direction we could take. The conditionals may need to be cleaned up to take into account discrete type media features like |
|
To work on the logic of the rule, you're best writing the tests first. Something along the lines of the following is a good start: testRule({
ruleName,
config: ['context'],
fix: true,
accept: [
{
code: '@media {}',
description: 'empty media query',
},
{
code: '@media () {}',
description: 'empty media feature',
},
{
code: '@media screen {}',
description: 'keyword',
},
{
code: '@media (color) {}',
description: 'range type media feature in boolean context',
},
{
code: '@media (pointer: fine) {}',
description: 'discrete type media feature',
},
{
code: '@media (width >= 1px) {}',
description: 'range type media feature in context notation',
},
{
code: '@media screen and (width >= 1px) {}',
description: 'range type media feature in context notation with keyword',
},
{
code: '@media not print, (width >= 1px) {}',
description: 'range type media feature in context notation in media query list',
},
{
code: '@media (1px <= width >= 2px) {}',
description: 'range type media feature in context notation with two values',
},
],
reject: [
{
code: '@media (min-width: 1px) {}',
description: 'range type media feature in prefix notation',
message: messages.expected('context'),
line: 1,
column: 12,
endLine: 1,
endColumn: 24,
},
{
code: '@media screen and (min-width: 1px) {}',
description: 'range type media feature in prefix notation with keyword',
message: messages.expected('context'),
line: 1,
column: 12,
endLine: 1,
endColumn: 24,
},
{
code: '@media not print, (min-width: 1px) {}',
description: 'range type media feature in prefix notation in media query list',
message: messages.expected('context'),
line: 1,
column: 12,
endLine: 1,
endColumn: 33,
},
],
});
testRule({
ruleName,
config: ['prefix'],
fix: true,
accept: [
{
code: '@media {}',
description: 'empty media query',
},
{
code: '@media () {}',
description: 'empty media feature',
},
{
code: '@media screen {}',
description: 'keyword',
},
{
code: '@media (color) {}',
description: 'range type media feature in boolean context',
},
{
code: '@media (pointer: fine) {}',
description: 'discrete type media query',
},
{
code: '@media (min-width: 1px) {}',
description: 'range type media feature in prefix notation',
},
{
code: '@media screen and (min-width: 1px) {}',
description: 'range type media feature in prefix notation with keyword',
},
{
code: '@media not print, (min-width: 1px) {}',
description: 'range type media feature in prefix notation in media query list',
},
],
reject: [
{
code: '@media (width >= 1px) {}',
description: 'range type media feature in context notation',
message: messages.expected('prefix'),
line: 1,
column: 12,
endLine: 1,
endColumn: 24,
},
{
code: '@media (1px < width >= 2px) {}',
description: 'range type media feature in context notation with two values',
message: messages.expected('prefix'),
line: 1,
column: 12,
endLine: 1,
endColumn: 24,
},
{
code: '@media screen and (width >= 1px) {}',
description: 'range type media feature in context notation with keyword',
message: messages.expected('prefix'),
line: 1,
column: 12,
endLine: 1,
endColumn: 24,
},
{
code: '@media not print, (width >= 1px) {}',
description: 'range type media feature in context notation in media query list',
message: messages.expected('prefix'),
line: 1,
column: 12,
endLine: 1,
endColumn: 33,
}
],
});(Note: positions aren't correct as this is something I quickly rustled up to show the type of code we should be testing against.) |
|
As per your suggestion, updated the code. Please have a look and guide me how to proceed from here or any other file also need to be updated with this change! |
| @@ -17,19 +17,27 @@ const lengthUnits = new Set([ | |||
| 'ric', | |||
| 'rlh', | |||
| // Viewport-percentage lengths | |||
| 'dvb', | |||
There was a problem hiding this comment.
#6428 has already been merged.
@sidverma32 Thank you for your progress so far with this pull request. I've picked up the work in #6497 as there were bits left to do that:
You can study the changes in #6497 to see what parts are needed for a new rule consistent with our existing rules. Thanks again for getting the ball rolling with this new rule. |
closes #6161
Name: media-feature-range-notation
Primary option: "prefix"|"context"
Secondary options: none
Autofixable: Yes, but only for the "context" option
Message: - Expected ${primary} media feature range notation
Description: "Specify prefix or context notation for media feature ranges."
Extended description: "Media features of the range type can be written using prefixes or the more modern context notation."
Section: "Enforce convention" -> "Media feature"