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

Fix false negatives for SVG keywords and add option: ["camelCaseSvgKeywords"] to value-keyword-case #5849

Merged
merged 13 commits into from
Jan 22, 2022
Merged

Fix false negatives for SVG keywords and add option: ["camelCaseSvgKeywords"] to value-keyword-case #5849

merged 13 commits into from
Jan 22, 2022

Conversation

kawaguchi1102
Copy link
Contributor

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

Closes #4884

Is there anything in the PR that needs further explanation?

Following the discussion in the issue thread, I have added a rule option.
I hope this will satisfy your request.

ybiquitous
ybiquitous reviewed Jan 19, 2022
View reviewed changes
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@kawaguchi1102 Thanks for creating the pull request!

[ask] @jeddy3 said on #4884 (comment):

We change the default behaviour so that the primary option (lower or upper) is used for every keyword, and add a camelCaseSvgKeywords: true option for users who want the existing behaviour.

so, when camelCaseSvgKeywords: false (this is the changed default behavior?), should currentcolor be always correct, not currentColor?

lib/rules/value-keyword-case/README.md Outdated Show resolved Hide resolved
lib/rules/value-keyword-case/README.md Outdated Show resolved Hide resolved
@jeddy3
Copy link
Member

jeddy3 commented Jan 20, 2022

so, when camelCaseSvgKeywords: false (this is the changed default behavior?), should currentcolor be always correct, not currentColor?

That's correct. This pull request should also fix the false negatives, i.e. currentColor, optimizeLegibility etc should be lowercase if "lower" is the primary option and if camelCaseSvgKeywords is set to false or not set at all.

jeddy3
jeddy3 requested changes Jan 20, 2022
View reviewed changes
Copy link
Member

@jeddy3 jeddy3 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@kawaguchi1102 Thanks for the pull request.

As @ybiquitous implied, the logic needs inverting i.e. when the camelCaseSvgKeywords option is set to true the SVG keywords should be camel case regardless of the primary option. When the it is set to false or not set, then the primary option, e.g. lower or upper, should be respected.

lib/rules/value-keyword-case/README.md Outdated Show resolved Hide resolved
lib/rules/value-keyword-case/README.md Outdated Show resolved Hide resolved
lib/rules/value-keyword-case/README.md Outdated Show resolved Hide resolved
@jeddy3 jeddy3 changed the title Add option: ["camelCaseSvgKeywords"] to value-keyword-case #4884 Fix false negatives for SVG keywords and add option: ["camelCaseSvgKeywords"] to value-keyword-case Jan 20, 2022
@jeddy3 jeddy3 mentioned this pull request Jan 20, 2022
Closed
6 tasks
@kawaguchi1102
Copy link
Contributor Author

Thank you for pointing out the mistake.
I was mistaken in my assumption.

I will fix this issue.

kawaguchi1102 and others added 9 commits January 21, 2022 00:56
@kawaguchi1102 @ybiquitous
Update lib/rules/value-keyword-case/README.md
fb5f811 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@kawaguchi1102 @ybiquitous
Update lib/rules/value-keyword-case/README.md
731eb0f 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@kawaguchi1102 @jeddy3
Update lib/rules/value-keyword-case/README.md
5c25764 
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@kawaguchi1102 @jeddy3
Update lib/rules/value-keyword-case/README.md
4c2801c 
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@kawaguchi1102 @jeddy3
Update lib/rules/value-keyword-case/README.md
244916f 
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@jeddy3
Update README.md
53589a1
@jeddy3
Update README.md
b113195
@jeddy3
Update index.js
341d984
@jeddy3
Update index.js
79a2ad2
jeddy3
jeddy3 approved these changes Jan 21, 2022
View reviewed changes
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.

Turns out the SVG keywords were only camelCase for the "lower" primary. I've aligned this new option with that, as well as patching up the logic and tests.

If true, this rule expects SVG keywords to be camel case when the primary option is "lower".

LGTM.

ybiquitous
ybiquitous approved these changes Jan 22, 2022
View reviewed changes
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

LGTM👍

@jeddy3 jeddy3 merged commit f3c2240 into stylelint:main Jan 22, 2022
@jeddy3
Copy link
Member

jeddy3 commented Jan 22, 2022

  • Added: camelCaseSvgKeywords to value-keyword-case - use this option if you want legacy camel case SVG keywords like currentColor (#5849).
  • Fixed: value-keyword-case false negatives for SVG keywords like currentcolor (#5849).

@bitstarr bitstarr mentioned this pull request Jan 24, 2022
Closed
@sync-by-unito sync-by-unito bot mentioned this pull request Jan 27, 2022
Closed
@sogame sogame mentioned this pull request Jan 27, 2022
Merged
@OndraM OndraM mentioned this pull request Jan 30, 2022
Merged
3 tasks
@janicekt janicekt mentioned this pull request Feb 28, 2022
Merged
@sync-by-unito sync-by-unito bot mentioned this pull request Mar 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeddy3 jeddy3 jeddy3 approved these changes

@ybiquitous ybiquitous ybiquitous approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add camelCaseSvgKeywords to value-keyword-case and set default to false
3 participants
@kawaguchi1102 @jeddy3 @ybiquitous