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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-synthesis
actually supports four longhand properties. However,font-synthesis-position
currently 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?