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

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

Conversation

5 participants
@evilebottnawi
Member

evilebottnawi commented Mar 2, 2017

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

This comment has been minimized.

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)

@evilebottnawi evilebottnawi force-pushed the issue-2406 branch from a92264a to 21f2976 Mar 2, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 2, 2017

@edg2s thanks, fixed

@edg2s

This comment has been minimized.

edg2s commented Mar 2, 2017

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

@@ -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")

This comment has been minimized.

@edg2s

edg2s Mar 2, 2017

Should be isCounterResetCustomIdentValue

@edg2s

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.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 2, 2017

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

@edg2s

This comment has been minimized.

edg2s commented Mar 2, 2017

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

@evilebottnawi evilebottnawi force-pushed the issue-2406 branch from 21f2976 to b26cff7 Mar 6, 2017

@evilebottnawi evilebottnawi changed the title from Fixed: `value-keyword-case` is now ignore counter-reset property. to 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

Looks good.

@hudochenkov

Great work! Thank you!

}, {
code: "a { content: counter(cOuNtEr-NaMe); }",
}, {
code: "a { content: counter(COUNTER-NAME); }",

This comment has been minimized.

@davidtheclark

davidtheclark Mar 7, 2017

Contributor

The same 3 tests are repeated, right?

This comment has been minimized.

@evilebottnawi

evilebottnawi Mar 7, 2017

Member

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

}, {
code: "a { content: counter(counter-name); }",
}, {
code: "a { content: counter(cOuNtEr-NaMe); }",

This comment has been minimized.

@davidtheclark

davidtheclark Mar 7, 2017

Contributor

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.

This comment has been minimized.

@evilebottnawi

evilebottnawi Mar 7, 2017

Member

@davidtheclark difference functions to ignore attr and counter

code: "a { content: oPeN-qUoTe; }",
message: messages.expected("oPeN-qUoTe", "OPEN-QUOTE"),
line: 1,
column: 14,

This comment has been minimized.

@davidtheclark

davidtheclark Mar 7, 2017

Contributor

How do these tests relate to the issue?

This comment has been minimized.

@evilebottnawi
@@ -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")) {

This comment has been minimized.

@davidtheclark

davidtheclark Mar 7, 2017

Contributor

How does attr relate to the issue?

This comment has been minimized.

@evilebottnawi

evilebottnawi Mar 7, 2017

Member

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

This comment has been minimized.

@davidtheclark

davidtheclark Mar 7, 2017

Contributor

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?

This comment has been minimized.

@evilebottnawi

evilebottnawi Mar 7, 2017

Member

@davidtheclark yep

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

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Mar 7, 2017

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

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 7, 2017

@davidtheclark A couple of minutes and send a commit 😄

@evilebottnawi evilebottnawi force-pushed the issue-2406 branch from a726845 to c2080f5 Mar 7, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 7, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Mar 7, 2017

@evilebottnawi Go ahead and merge!

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 7, 2017

@davidtheclark merge? 😄

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Mar 7, 2017

Yes, merge!

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Mar 7, 2017

To merge or not to merge...

@evilebottnawi evilebottnawi merged commit 88e6bc1 into master Mar 11, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@evilebottnawi evilebottnawi deleted the issue-2406 branch Mar 11, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 11, 2017

Added to Changelog:

  • Fixed: false positives for attr, counter, counters functions and counter-reset property in value-keyword-case (#2407).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment