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 false negatives for SVG keywords and add option: ["camelCaseSvgKeywords"] to value-keyword-case #5849

Merged
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.

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.

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.

@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
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
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
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.

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).

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.

Add camelCaseSvgKeywords to value-keyword-case and set default to false
3 participants