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
added optional group name property and used it for error messages #73
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
As I said before, logic should be located in other place.
Inside this block you can add group name for every property, if grope name is present:
stylelint-order/rules/properties-order/createExpectedOrder.js
Lines 15 to 26 in 3215ac3
if (_.isString(item)) { | |
// In flexible groups, the expectedPosition does not ascend | |
// to make that flexibility work; | |
// otherwise, it will always ascend | |
if (!inFlexibleGroup) { | |
expectedPosition += 1; | |
} | |
order[item] = { separatedGroup, expectedPosition }; | |
return; | |
} |
Then pass groupName to:
stylelint-order/rules/properties-order/checkNode.js
Lines 51 to 54 in 3215ac3
message: sharedInfo.messages.expected( | |
checkedOrder.secondNode.name, | |
checkedOrder.firstNode.name | |
), |
Please, add tests and documentation.
rules/properties-order/index.js
Outdated
expected: (first, second, groupName) => ` | ||
Expected "${first}"${ | ||
groupName ? ` to be grouped with other "${groupName}" properties and` : '' | ||
} to come before "${second}"`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, use @jeddy3's suggestion:
Expected "${first}" to come before "${second}" in group "${groupName}"
Conditionally add “in group...”.
06b7999
to
f8a8307
Compare
f8a8307
to
03228aa
Compare
I refactored the code and added tests and documentation. I thought it would be good for the readme to include at least one example of using |
Looks good! Thanks! |
Thanks for merging. Note that I didn't bump the version number—I was leaving it up to you to decide about that. Do you expect to publish a new version of the npm package sometime soon? |
I want to include a fix for #74 in the next version. I'll most likely do it during this week. |
Released in |
Great, thank you :) |
Resolves #72