feat: add relative-font-units rule
#133
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a new ESLint rule, "relative-font-units", which enforces the use of relative units for font size. Key changes include:
- A new rule implementation in src/rules/relative-font-units.js with support for custom allowed units.
- Updates to the plugin's index file, enabling the new rule.
- Documentation updates in docs/rules/relative-font-units.md and README.md to describe and list the rule.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/rules/relative-font-units.js | New rule logic enforcing allowed relative font units. |
| src/index.js | Updated plugin registry to include the new rule. |
| docs/rules/relative-font-units.md | Documentation for the new rule including background and examples. |
| README.md | Rule table updated to list the new relative-font-units rule. |
src/rules/relative-font-units.js
Outdated
| return { | ||
| Declaration(node) { | ||
| if (node.property === "font-size") { | ||
| if (node.value.type === "Value") { |
There was a problem hiding this comment.
Accessing the first element of node.value.children without checking if the array is non-empty may lead to runtime errors. Consider verifying that the children array has at least one element before accessing it.
Suggested change
| if (node.value.type === "Value") { | |
| if (node.value.type === "Value" && node.value.children.length > 0) { |
Copilot uses AI. Check for mistakes.
src/rules/relative-font-units.js
Outdated
Comment on lines
137
to
154
| if (!allowedFontUnits.includes("%") && percentageNode) { | ||
| shouldReport = true; | ||
| location = percentageNode.loc; | ||
| } | ||
|
|
||
| if (identifierNode) { | ||
| shouldReport = true; | ||
| location = identifierNode.loc; | ||
| } | ||
|
|
||
| if ( | ||
| dimensionNode && | ||
| !allowedFontUnits.includes(dimensionNode.unit) | ||
| ) { | ||
| shouldReport = true; | ||
| location = dimensionNode.loc; | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The condition chain in the 'font' property branch is somewhat complex. Consider refactoring the conditions to improve readability and maintainability.
Suggested change
| if (!allowedFontUnits.includes("%") && percentageNode) { | |
| shouldReport = true; | |
| location = percentageNode.loc; | |
| } | |
| if (identifierNode) { | |
| shouldReport = true; | |
| location = identifierNode.loc; | |
| } | |
| if ( | |
| dimensionNode && | |
| !allowedFontUnits.includes(dimensionNode.unit) | |
| ) { | |
| shouldReport = true; | |
| location = dimensionNode.loc; | |
| } | |
| const conditions = [ | |
| { | |
| check: !allowedFontUnits.includes("%") && percentageNode, | |
| loc: percentageNode?.loc, | |
| }, | |
| { | |
| check: identifierNode, | |
| loc: identifierNode?.loc, | |
| }, | |
| { | |
| check: dimensionNode && !allowedFontUnits.includes(dimensionNode.unit), | |
| loc: dimensionNode?.loc, | |
| }, | |
| ]; | |
| for (const condition of conditions) { | |
| if (condition.check) { | |
| shouldReport = true; | |
| location = condition.loc; | |
| break; | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
nzakas
reviewed
May 16, 2025
| } | ||
| ``` | ||
|
|
||
| ## When Not to Use It |
There was a problem hiding this comment.
Can you add this link in a Further Reading section?
https://www.joshwcomeau.com/css/surprising-truth-about-pixels-and-accessibility/
lumirlumir
added
the
accepted
nzakas
previously approved these changes
May 19, 2025
docs/rules/relative-font-units.md
Outdated
| ``` | ||
|
|
||
| ## When Not to Use It | ||
|
|
||
| If your project does not prioritize the use of relative font-size units—such as in cases requiring absolute sizing for print styles, pixel-perfect UI components, or embedded widgets—you may safely disable this rule without impacting your intended design precision. | ||
|
|
||
| ## Prior Art |
There was a problem hiding this comment.
Suggested change
| ## Prior Art | |
| ## Further Reading |
docs/rules/relative-font-units.md
Outdated
Comment on lines
9
to
11
| 1. Keywords (e.g., small, medium, large). | ||
| 1. Length units (e.g., px, em, rem, pt). | ||
| 1. Percentages (%, relative to the parent element's font size). |
There was a problem hiding this comment.
Suggested change
| 1. Keywords (e.g., small, medium, large). | |
| 1. Length units (e.g., px, em, rem, pt). | |
| 1. Percentages (%, relative to the parent element's font size). | |
| 1. Keywords (e.g., `small`, `medium`, `large`). | |
| 1. Length units (e.g., `px`, `em`, `rem`, `pt`). | |
| 1. Percentages (`%`, relative to the parent element's font size). |
nzakas
previously approved these changes
May 20, 2025
snitin315
reviewed
May 20, 2025
docs/rules/relative-font-units.md
Outdated
|
|
||
| This rule enforces the use of relative units for font size. | ||
|
|
||
| ### Options |
There was a problem hiding this comment.
I think it should be h2.
Suggested change
| ### Options | |
| ## Options |
docs/rules/relative-font-units.md
Outdated
| 1. Length units (e.g., `px`, `em`, `rem`, `pt`). | ||
| 1. Percentages (`%`, relative to the parent element's font size). | ||
|
|
||
| Generally, relative unit such as `rem` or `em` are preferred over absolute ones like `px`, `pt` because of following reasons: |
There was a problem hiding this comment.
Suggested change
| Generally, relative unit such as `rem` or `em` are preferred over absolute ones like `px`, `pt` because of following reasons: | |
| Generally, relative units such as `rem` or `em` are preferred over absolute ones like `px`, `pt` because of the following reasons: |
docs/rules/relative-font-units.md
Outdated
|
|
||
| ### Options | ||
|
|
||
| This rule accepts an option which is an object with following property: |
There was a problem hiding this comment.
Suggested change
| This rule accepts an option which is an object with following property: | |
| This rule accepts an option which is an object with the following property: |
docs/rules/relative-font-units.md
Outdated
|
|
||
| This rule accepts an option which is an object with following property: | ||
|
|
||
| - `allow` (default: `["rem"]`) - Specify an array of relative units that are allowed to be used. You can use following units: |
There was a problem hiding this comment.
Suggested change
| - `allow` (default: `["rem"]`) - Specify an array of relative units that are allowed to be used. You can use following units: | |
| - `allow` (default: `["rem"]`) - Specify an array of relative units that are allowed to be used. You can use the following units: |
|
|
||
| Example of **incorrect** code for default `{ allow: ["rem"] }` option: | ||
|
|
||
| ```css |
There was a problem hiding this comment.
Suggested change
| ```css | |
| ```css | |
| /* eslint css/relative-font-units: ["error", { allow: ["rem"] }] */ | |
docs/rules/relative-font-units.md
Outdated
Comment on lines
62
to
63
| ```css | ||
| a { |
There was a problem hiding this comment.
Suggested change
| ```css | |
| a { | |
| ```css | |
| /* eslint css/relative-font-units: ["error", { allow: ["rem"] }] */ | |
| a { |
docs/rules/relative-font-units.md
Outdated
Comment on lines
81
to
82
| ```css | ||
| a { |
There was a problem hiding this comment.
Suggested change
| ```css | |
| a { | |
| ```css | |
| /* eslint css/relative-font-units: ["error", { allow: ["em", "%"] }] */ | |
| a { |
snitin315
reviewed
May 20, 2025
snitin315
reviewed
May 20, 2025
snitin315
reviewed
May 20, 2025
snitin315
reviewed
May 20, 2025
Comment on lines
+29
to
+30
| "a { font: 2rem Arial, sans-serif; }", | ||
| "a { font: 1.2rem/2 Arial, sans-serif; }", |
There was a problem hiding this comment.
Let's add test cases for all possible syntax for the font shorthand. https://developer.mozilla.org/en-US/docs/Web/CSS/font#syntax
nzakas
approved these changes
May 21, 2025
There was a problem hiding this comment.
Re-approving. Waiting for @snitin315 to review before merging.
github-project-automation
bot
moved this from Second Review Needed
to Complete
in Triage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisites checklist
What is the purpose of this pull request?
Add a new rule called
relative-font-unitsthat will enforce the relative units for font size.What changes did you make? (Give an overview)
Added the rule
relative-font-unitswith an option calledallowand related tests and docs and updated README.md file.Related Issues
fixes #73
Is there anything you'd like reviewers to focus on?