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.

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 column position and ignore Sass function for font-weight-notation #6005

Merged
merged 2 commits into from Apr 4, 2022

Conversation

ybiquitous
Copy link
Member

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

Is there anything in the PR that needs further explanation?

This change uses postcss-value-parser instead of postcss.list.space() for more accurate parsing.


@font-face { font-weight: normal bold; }
/** ↑
* Multiple notations are available in @font-face */
Copy link
Member Author

Choose a reason for hiding this comment

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

[note] I think adding a description about @font-face makes sense. Any thoughts?

See also https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/font-weight

@@ -180,7 +180,7 @@ testRule({
code: '@font-face { font-weight: 400 bold; }',
message: messages.expected('numeric'),
line: 1,
column: 27,
column: 31,
Copy link
Member Author

Choose a reason for hiding this comment

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

[note] The position is changed to bold, not 400.

return valueParser(value).nodes.filter((node, index, nodes) => {
if (node.type !== 'word') return false;

// Exclude `<font-size>/<line-height>` format like `16px/3`.
Copy link
Member Author

Choose a reason for hiding this comment

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

*/
function complain(message) {
function complain(message, valueNode) {
const index = declarationValueIndex(decl) + (valueNode ? valueNode.sourceIndex : 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

[note] I'll add endIndex on #5725.

@ybiquitous ybiquitous marked this pull request as ready for review April 3, 2022 10:59
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Fantastic, thank you!

This is a nice stepping stone to #3158 (and also jeddy3/stylelint-scales#73).

I've asked one question.

if (decl.prop.toLowerCase() === 'font-weight') {
checkWeight(decl.value, decl);
}
root.walkDecls(/^font(-weight)?$/i, (decl) => {
Copy link
Member

Choose a reason for hiding this comment

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

👍

lib/rules/font-weight-notation/index.js Outdated Show resolved Hide resolved
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@ybiquitous ybiquitous merged commit 4b4e792 into main Apr 4, 2022
@ybiquitous ybiquitous deleted the fix-font-weight-notation branch April 4, 2022 09:42
@ybiquitous
Copy link
Member Author

@jeddy3 Thank you for the review! I've updated the changelog:

  • Fixed: font-weight-notation false positives for Sass functions and column position (#6005).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix false positives for functions in font-weight-notation
2 participants