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

Fixed: value-keyword-case is now ignore counter-reset property and counter, counters and attr functions. #2407

Merged
merged 2 commits into from
Mar 11, 2017
Merged

Fixed: value-keyword-case is now ignore counter-reset property and counter, counters and attr functions. #2407

merged 2 commits into from
Mar 11, 2017

Conversation

alexander-akait
Copy link
Member

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

#2406

Is there anything in the PR that needs further explanation?

e.g. "No, it's self explanatory."

@edg2s
Copy link

edg2s commented Mar 2, 2017

Looks good. You should add duplicate tests to the "upper" blocks (i.e. wherever we currently have counter-increment tests)

@alexander-akait
Copy link
Member Author

@edg2s thanks, fixed

@edg2s
Copy link

edg2s commented Mar 2, 2017

#2408 fixes the other part of this bug (counter functions)

@alexander-akait alexander-akait mentioned this pull request Mar 2, 2017
Closed
edg2s
edg2s reviewed Mar 2, 2017
View reviewed changes
lib/rules/value-keyword-case/index.js Outdated
@@ -4,6 +4,7 @@ const keywordSets = require("../../reference/keywordSets")
const declarationValueIndex = require("../../utils/declarationValueIndex")
const getUnitFromValueNode = require("../../utils/getUnitFromValueNode")
const isCounterIncrementCustomIdentValue = require("../../utils/isCounterIncrementCustomIdentValue")
const isCounterResetCustomIdentValue = require("../../utils/isCounterIncrementCustomIdentValue")
Copy link

Choose a reason for hiding this comment

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

Should be isCounterResetCustomIdentValue

edg2s
edg2s suggested changes Mar 2, 2017
View reviewed changes
Copy link

@edg2s edg2s left a comment

Choose a reason for hiding this comment

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

Typo in new "require".

One could merge counterIncrementKeywords/counterResteKeywords as they are identical in the spec. Also in CSS3 where counter-set is introduced. Or we could keep them separate.

@alexander-akait
Copy link
Member Author

@edg2s best could keep them separate. They can change late, imho.

@edg2s
Copy link

edg2s commented Mar 2, 2017

Ok - you still have a typo in the 'require' statement.

@alexander-akait alexander-akait changed the title Fixed: value-keyword-case is now ignore counter-reset property. Fixed: value-keyword-case is now ignore counter-reset property and counter, counters and attr functions. Mar 6, 2017
edg2s
edg2s approved these changes Mar 6, 2017
View reviewed changes
Copy link

@edg2s edg2s left a comment

Choose a reason for hiding this comment

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

Looks good.

hudochenkov
hudochenkov approved these changes Mar 7, 2017
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.

Great work! Thank you!

davidtheclark
davidtheclark reviewed Mar 7, 2017
View reviewed changes
lib/rules/value-keyword-case/__tests__/index.js
}, {
code: "a { content: counter(cOuNtEr-NaMe); }",
}, {
code: "a { content: counter(COUNTER-NAME); }",
Copy link
Contributor

Choose a reason for hiding this comment

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

The same 3 tests are repeated, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark typo, it should be counters (ending on s)

lib/rules/value-keyword-case/__tests__/index.js
}, {
code: "a { content: counter(counter-name); }",
}, {
code: "a { content: counter(cOuNtEr-NaMe); }",
Copy link
Contributor

Choose a reason for hiding this comment

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

If you've already tested the line about toLowerCase (with the value-string tests above), I don't think we need to multiply every test with alternate cases.

Copy link
Member Author

@alexander-akait alexander-akait Mar 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@davidtheclark difference functions to ignore attr and counter

lib/rules/value-keyword-case/__tests__/index.js Outdated
code: "a { content: oPeN-qUoTe; }",
message: messages.expected("oPeN-qUoTe", "OPEN-QUOTE"),
line: 1,
column: 14,
Copy link
Contributor

Choose a reason for hiding this comment

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

How do these tests relate to the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark yep extra tests

lib/rules/value-keyword-case/index.js
@@ -64,7 +65,7 @@ const rule = function (expectation, options) {
}

// Ignore keywords within `url` and `var` function
if (node.type === "function" && (valueLowerCase === "url" || valueLowerCase === "var")) {
if (node.type === "function" && (valueLowerCase === "url" || valueLowerCase === "var" || valueLowerCase === "counter" || valueLowerCase === "counters" || valueLowerCase === "attr")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How does attr relate to the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark I thought that I would correct this behavior in one commit

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. In the future please make sure to point this out in the comment about your PR, so we remember to place it in the Changelog. Is it tested?

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark yep

stylelint/lib/rules/value-keyword-case/__tests__/index.js

Line 63 in a726845

code: "a { content: attr(value-string); }",

@davidtheclark
Copy link
Contributor

@evilebottnawi Great! After you make those couple of changes, want to merge and add a changelog entry?

@alexander-akait
Copy link
Member Author

@davidtheclark A couple of minutes and send a commit 😄

@alexander-akait
Copy link
Member Author

/cc @davidtheclark

@davidtheclark
Copy link
Contributor

@evilebottnawi Go ahead and merge!

@alexander-akait
Copy link
Member Author

@davidtheclark merge? 😄

@davidtheclark
Copy link
Contributor

Yes, merge!

@hudochenkov
Copy link
Member

To merge or not to merge...

@alexander-akait alexander-akait merged commit 88e6bc1 into master Mar 11, 2017
@alexander-akait alexander-akait deleted the issue-2406 branch March 11, 2017 13:46
@alexander-akait alexander-akait mentioned this pull request Mar 11, 2017
Closed
@alexander-akait
Copy link
Member Author

Added to Changelog:

  • Fixed: false positives for attr, counter, counters functions and counter-reset property in value-keyword-case (#2407).

@renovate-bot renovate-bot mentioned this pull request Apr 1, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtheclark davidtheclark davidtheclark left review comments

@hudochenkov hudochenkov hudochenkov approved these changes

@edg2s edg2s edg2s approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alexander-akait @edg2s @davidtheclark @hudochenkov @jeddy3