Fix declaration-block-no-redundant-longhand-properties false negative for font-synthesis
#7214
Conversation
…ve for `font-synthesis`
🦋 Changeset detectedLatest commit: 9c8d9c5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
@mattxwang Thanks for creating the pull request!
Not supporting font-synthesis-position for now makes sense to me. We can revisit it later if a request comes. 👍🏼
I've posted a few minor suggestion comments, but they are my preference. So you can ignore them (please resolve them if you won't address them).
lib/rules/declaration-block-no-redundant-longhand-properties/index.js
Outdated
Show resolved
Hide resolved
lib/rules/declaration-block-no-redundant-longhand-properties/index.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
| if ( | ||
| !isValidFontSynthesisValue(weight) || | ||
| !isValidFontSynthesisValue(style) || | ||
| !isValidFontSynthesisValue(smallCaps) | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I realized that earlier, this line was uncovered. I've added a test that involves some invalid CSS (in 8fddced) - is this the right way to go about this? For context, this branch is really only checking for invalid CSS (if the values for each of the longhands are not auto or none, which are the only allowed values in the spec)
(I noticed some of the earlier test cases also involve slightly malformed CSS, ex the transition test cases)
If not, I can remove the test case and/or try something else?
There was a problem hiding this comment.
which are the only allowed values in the spec
I think that global values are also accepted.
i.e. inherit; initial; revert; revert-layer; unset;
I wonder if decls.get retrieves it raw or if it does the mapping automatically.
i.e. is font-synthesis-weight: initial; considered invalid by this implementation? (maps to auto hence valid)
There was a problem hiding this comment.
I think the added test case is enough to cover the path. 👍🏼
And supporting global values is ideal, but I don't think it's required for now. We can revisit it later if people request.
There was a problem hiding this comment.
We can revisit it later if people request.
It should be done for all properties at once; let's leave it for a later enhancement.
Ref: #7213 (comment) (and transitively, #7200 (comment)).
First, do we like this custom resolver? It is a bit long!
Secondly,
font-synthesisactually supports four longhand properties. However,font-synthesis-positioncurrently has 0.41% adoption according to caniuse, which is far too small for it to be relevant. I've elected to not include it for now; however, we could also include it if we think that better future-proofs the rule?