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.

Sign up for GitHub

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

Jump to bottom

Add autofix to font-weight-notation #3159

Closed
wants to merge 1 commit into from
Closed

Add autofix to font-weight-notation #3159

wants to merge 1 commit into from

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented Feb 7, 2018

Related Issue

#3158

Description

we enable fix option for font-weight-notation.

numeric

a { font-weight: normal; } -> a { font-weight: 400; }
a { font: italic bold 20px; } -> a { font: italic 700 20px; }

named-where-possible

a { font-weight: 400; } -> a { font-weight: normal; }
a { font: italic 700 20px; } -> a { font: italic bold 20px; }

@alexander-akait alexander-akait added PR: needs documentation type: enhancement a new feature that isn't related to rules labels Feb 7, 2018
alexander-akait
alexander-akait reviewed Feb 7, 2018
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good work! Thanks! Need update docs, look how we do this for other rules

@hiroppy hiroppy force-pushed the feature/make-use-of-fix-option-for-font-weight-notation branch from 2977230 to 045da2d Compare February 7, 2018 16:14
@hiroppy
Copy link
Member Author

hiroppy commented Feb 7, 2018

updated docs;)

hiroppy
hiroppy commented Feb 7, 2018
View reviewed changes
lib/rules/font-weight-notation/index.js Outdated
if (optionName === "numeric") {
return input
.replace(/normal/g, WEIGHTS_WITH_KEYWORD_EQUIVALENTS[0])
.replace(/bold/g, WEIGHTS_WITH_KEYWORD_EQUIVALENTS[1]);
Copy link
Member Author

Choose a reason for hiding this comment

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

This line is not good... I'll fix.(e.g. font: var(--bold-x) italic 20px;)

Copy link
Member

Choose a reason for hiding this comment

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

@abouthiroppy Can you add tests and fix this?

Copy link
Member Author

@hiroppy hiroppy Feb 7, 2018 •
edited

Choose a reason for hiding this comment

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

Sure. BTW, Should we also support auto fix other than CSS?(maybe sass, cssnext, etc..)

Copy link
Member

Choose a reason for hiding this comment

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

@abouthiroppy where only standard syntax

@hiroppy hiroppy force-pushed the feature/make-use-of-fix-option-for-font-weight-notation branch 2 times, most recently from 77c8cca to a88eee6 Compare February 7, 2018 18:23
@hiroppy
Copy link
Member Author

hiroppy commented Feb 7, 2018 •
edited

@evilebottnawi PTAL;)
Added https://github.com/stylelint/stylelint/pull/3159/files#diff-4f270c4864d2cee8f34ebc01d38d21e6R345 and https://github.com/stylelint/stylelint/pull/3159/files#diff-4f270c4864d2cee8f34ebc01d38d21e6R387, and changed core code's logic.

hudochenkov
hudochenkov requested changes Feb 20, 2018
View reviewed changes
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Thank you for starting your work on autofixing.

When autofixing is added to a rule all testRule() should have fix: true. Not only new tests.

Please, add fix: true to every testRule() for this rule.

@jeddy3 jeddy3 removed the type: enhancement a new feature that isn't related to rules label Feb 22, 2018
@jeddy3 jeddy3 changed the title Enable fix option for font-weight-notation Add autofix to font-weight-notation Mar 15, 2018
@hudochenkov
Copy link
Member

@hiroppy Friendly ping to see if you're still working on this PR?

@hiroppy
Copy link
Member Author

hiroppy commented Jul 7, 2018

@hudochenkov Sorry for the late reply. I will update this PR soon;)

@hiroppy hiroppy force-pushed the feature/make-use-of-fix-option-for-font-weight-notation branch from a88eee6 to 91372ca Compare July 7, 2018 23:33
@hiroppy
Copy link
Member Author

hiroppy commented Jul 7, 2018 •
edited

@hudochenkov PTAL

Sorry, I found some bugs so I'll fix it.

hudochenkov
hudochenkov reviewed Jul 8, 2018
View reviewed changes
lib/rules/font-weight-notation/__tests__/index.js
@@ -6,6 +6,7 @@ const { messages, ruleName } = rule;
testRule(rule, {
ruleName,
config: ["numeric"],
fixed: true,
Copy link
Member

Choose a reason for hiding this comment

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

It should be fix: true. And all four testRule should have it.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, I fixed.

Copy link
Member

Choose a reason for hiding this comment

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

It's not fixed. I see fixed: true in ever testRule. It should be fix: true. Otherwise tests for autofixing don't executed.

@ntwb
Copy link
Member

ntwb commented Aug 19, 2018

It looks like all outstanding issues have been resolved.

Wondering if a note should be added to rule readme noting that only 400/normal and 700/bold are the only two autofixable?

@alexander-schranz
Copy link
Contributor

Any update to this?

@alexander-akait
Copy link
Member

/cc @hiroppy can you rebase?

@hudochenkov
Copy link
Member

Also, this is not resolved yet.

@hudochenkov
Copy link
Member

Closing due inactivity. We can reopen if requested changes were made.

@mattxwang
Copy link
Member

Was leafing through old issues / PRs and saw this - I'm more than happy to branch off of @hiroppy's changes (while still crediting them) and then implementing the requested changes - any thoughts @hudochenkov?

@hudochenkov
Copy link
Member

@mattxwang We probably need to use another solution, because we don't want to use style-search anymore. If you start working on this, start your branch from v14 branch.

@jeddy3
Copy link
Member

jeddy3 commented Jun 5, 2021

@mattxwang yep, as @hudochenkov said, we'll want to avoid using style-search. I suggest migrating to the value parser as detailed here #3855 (comment), that'll close that bug and once the value parser is in place being able to autofix the nodes should be trival.

@jeddy3 jeddy3 mentioned this pull request Mar 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hudochenkov hudochenkov hudochenkov requested changes

@alexander-akait alexander-akait alexander-akait left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@hiroppy @hudochenkov @ntwb @alexander-schranz @alexander-akait @mattxwang @jeddy3