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
[stylelint-polaris] Custom stylelint messages #7696
Conversation
size-limit report 📦
|
40aebf9
to
9e04887
Compare
dde827d
to
ac91bc0
Compare
8af0742
to
2d11e99
Compare
9e04887
to
3aaeced
Compare
118212d
to
efbc3ad
Compare
7e14397
to
a2eafba
Compare
bd66302
to
5059cc7
Compare
categoryRuleSeverity: categoryRuleSettings?.[1]?.severity, | ||
categoryRuleFix: | ||
context.fix && !categoryRuleSettings?.[1]?.disableFix, | ||
const ruleName = `polaris/${category}/${deNamespaceRuleName( |
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.
question: Why are we removing the namespace? This is how custom rules are distinguished from the Stylelint built-ins (e.g. polaris/at-rule-disallowed-list vs Stylelint's at-rule-disallowed-list)
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.
We don't remove from the plugin itself, so as not to conflict when passing the stylelintRuleName
to checkAgainstRule
. But when reporting the problem, we don't want the polaris
namespace in there twice:
- polaris/colors/polaris/at-rule-disallowed-list
+ polaris/colors/at-rule-disallowed-list
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.
Reverting as it looks like the VS Code diagnostic QuickFix actions won't work if the duplicate namespace is removed, as the disables comments are based on the names of the rules as they are enabled not on the names of the rules as they're reported 😢
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.
Gotcha, yea I have a slight preference to keep namespaces to continue Stylelint's conventions of disambiguating built-in rules from custom rules. Also, to avoid (future) rule name conflicts and ensuring a stylelint-disable
comment like the following is possible:
// stylelint-disable polaris/colors/at-rule-disallowed-list, polaris/colors/polaris/at-rule-disallowed-list
Totally, understanding wanting to dedupe polaris
though
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.
Ah yeah, that makes total sense 💯
9dda97c
to
05e1c72
Compare
@@ -4,414 +4,503 @@ const { | |||
tokens, | |||
} = require('@shopify/polaris-tokens'); | |||
|
|||
/** @type {import('./plugins/coverage').PrimaryOptions} */ |
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.
suggestion: Can we keep this type named PrimaryOptions
? This was intentionally aligned with terminology in the Stylelint docs and Type definitions.
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.
💯
stylelint-polaris/index.js
Outdated
/([\w-]+\.)?border-radius/, | ||
/([\w-]+\.)?border-width/, | ||
/([\w-]+\.)?border/, |
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.
thought: I see this is copy/paste, but wonder why there's no ($|\()
suffix?
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.
Ah great catch!! Those are functions not mixins. So they actually need to be in function-disallowed-list
, which only matches invocations of whatever is provided is provided in its primary options.
fix: context.fix && !stylelintRuleConfig?.[1]?.disableFix, | ||
appendedMessage: | ||
stylelintRuleConfig?.[1]?.message || categorySettings?.message, | ||
meta: categorySettings?.meta || defaultMeta, |
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.
question: Is it possible to allow individual rules to define their own meta
? Might be neat in some cases to provide links directly to the README.md#<category>-<rule-name>
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.
We can link there without it being stored in secondary actions by concatenating stylelintRuleName
to the category meta URL 💯
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.
This actually doesn't work unfortunately. After testing it, because the rule names are duplicated across categories, they have URLs like
https://github.com/Shopify/polaris/blob/stylelint-polaris-rule-docs/stylelint-polaris/README.md#declaration-property-value-disallowed-list-5
So we can't create the right URL just from the rule name alone.
const coverageRuleName = 'polaris/coverage'; | ||
|
||
/** | ||
* @typedef {import('stylelint').ConfigRules} StylelintRuleConfig |
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.
* @typedef {import('stylelint').ConfigRules} StylelintRuleConfig | |
* @typedef {import('stylelint').ConfigRules} StylelintConfigRules |
Light suggestion
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.
💯
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.
This is great! Nice job getting the Stylelint/VS Code link working 😍
85e1adc
to
625db06
Compare
/** | ||
* Returns the arguments expected by Stylelint rules that support functional custom messages | ||
* @param {string} ruleName The category's default message | ||
* @param {import('postcss').Node} node The node being reported as a problem | ||
* @returns {Parameters<import('stylelint').RuleMessageFunc> | undefined} An array of arguments for stylelint.report to invoke the functional message with | ||
*/ | ||
function getMessageArgs(ruleName, node) { | ||
if (!node) return undefined; | ||
|
||
const stylelintRuleMessageArgs = { | ||
'color-no-hex': [node.value], | ||
'function-disallowed-list': [node.value], | ||
'at-rule-disallowed-list': [node.name, node.params], | ||
'property-disallowed-list': [node.prop], | ||
'declaration-property-value-disallowed-list': [node.prop, node.value], | ||
'declaration-property-unit-disallowed-list': [node.prop, node.value], | ||
}; | ||
|
||
return stylelintRuleMessageArgs[ruleName]; | ||
} | ||
|
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.
question: Should this be removed now that we always append custom messages?
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.
Yes!
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.
Removed in this PR as I forgot to remove for this one 👀
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.
Nice!!! Messages are sweet 🚀
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@10.16.0 ### Minor Changes - [#7893](#7893) [`9e617c4d9`](9e617c4) Thanks [@yurm04](https://github.com/yurm04)! - Updating TextField and Select border colors to be compliant with accessibility color contrast guidance - [#7910](#7910) [`5a4faf821`](5a4faf8) Thanks [@henryyi](https://github.com/henryyi)! - Added support for hover delay before displaying tooltip ### Patch Changes - [#7890](#7890) [`53d836dc2`](53d836d) Thanks [@mrcthms](https://github.com/mrcthms)! - Fixed display bug with `BulkActions` when content in the table changes at the same time the bulk actions bar is visible - [#7846](#7846) [`65131df18`](65131df) Thanks [@qt314](https://github.com/qt314)! - - Changed border radius of `focus-ring` to use shape tokens instead of adding an extra `1px` - Updated `TextField` placeholder text to not change to a custom color on error - Delete an unused class in the `DataTable` and `IndexTable` - Replace some `px` values with tokens - Add some stylelint ignore context comments - [#7891](#7891) [`ff82bdb18`](ff82bdb) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated the colors of the borders of the buttons in the `BulkActions` component ## @shopify/plugin-polaris@0.0.21 ### Patch Changes - Updated dependencies \[]: - @shopify/polaris-migrator@0.10.1 ## @shopify/polaris-migrator@0.10.1 ### Patch Changes - Updated dependencies \[[`4b96147b3`](4b96147), [`c8a294f51`](c8a294f), [`6cd75fd9a`](6cd75fd), [`bdf04a289`](bdf04a2), [`9206b7b13`](9206b7b)]: - @shopify/stylelint-polaris@5.0.1 ## @shopify/stylelint-polaris@5.0.1 ### Patch Changes - [#7866](#7866) [`4b96147b3`](4b96147) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Updated `stylelint-polaris` to allow disabling specific coverage rules instead of entire categories - [#7906](#7906) [`c8a294f51`](c8a294f) Thanks [@qt314](https://github.com/qt314)! - Fixed config atRule and function regex to match whole word - [#7868](#7868) [`6cd75fd9a`](6cd75fd) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Update `package.json` to define a single entry point - [#7696](#7696) [`bdf04a289`](bdf04a2) Thanks [@chloerice](https://github.com/chloerice)! - Implemented custom message configuration support for polaris/coverage plugin - [#7898](#7898) [`9206b7b13`](9206b7b) Thanks [@qt314](https://github.com/qt314)! - Bug fix to wrap z-index 'declaration-property-value-allowed-list' token value with "var" ## polaris.shopify.com@0.27.2 ### Patch Changes - Updated dependencies \[[`53d836dc2`](53d836d), [`9e617c4d9`](9e617c4), [`5a4faf821`](5a4faf8), [`65131df18`](65131df), [`ff82bdb18`](ff82bdb)]: - @shopify/polaris@10.16.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Closes Shopify#7506 - [x] Bring in changes from https://github.com/Shopify/polaris/pull/7814/files - [ ] ~Finish adding examples to coverage/README.md~ (handling in separate [PR](Shopify#7878)) - [x] Update screenshots to reflect current approach - [x] Update/add comments to notable changes/fixes/open questions This pull request adds support for configuring `stylelint-polaris/coverage` rules with custom messages and metadata, so that error and warning messages tell admin builders how to resolve the problem and VS Code diagnostics link to the `@shopify/stylelint-polaris` documentation. For example: <img width="610" alt="Screenshot 2022-12-09 at 1 41 40 PM" src="https://user-images.githubusercontent.com/18447883/206791342-cc12ccab-9d36-44db-a619-d7c59dd2b265.png"> This PR also includes a bit of polish: - Added typing and descriptions to `polaris/coverage` plugin so that primary options are clear in the config and in the plugin's code - Updated plugin names to be singular instead of plural ```diff - custom-properties-allowed-list + custom-property-allowed-list - media-queries-allowed-list + media-query-allowed-list ``` <!-- Summary of the changes committed. Before / after screenshots are appreciated for UI changes. Make sure to include alt text that describes the screenshot. If you include an animated gif showing your change, wrapping it in a details tag is recommended. Gifs usually autoplay, which can cause accessibility issues for people reviewing your PR: <details> <summary>Summary of your gif(s)</summary> <img src="..." alt="Description of what the gif shows"> </details> --> <!-- ℹ️ Delete the following for small / trivial changes --> 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) <!-- Give as much information as needed to experiment with the component in the playground. --> <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React from 'react'; import {Page} from '../src'; export function Playground() { return ( <Page title="Playground"> {/* Add the code you want to test in here */} </Page> ); } ``` </details> - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@10.16.0 ### Minor Changes - [Shopify#7893](Shopify#7893) [`9e617c4d9`](Shopify@9e617c4) Thanks [@yurm04](https://github.com/yurm04)! - Updating TextField and Select border colors to be compliant with accessibility color contrast guidance - [Shopify#7910](Shopify#7910) [`5a4faf821`](Shopify@5a4faf8) Thanks [@henryyi](https://github.com/henryyi)! - Added support for hover delay before displaying tooltip ### Patch Changes - [Shopify#7890](Shopify#7890) [`53d836dc2`](Shopify@53d836d) Thanks [@mrcthms](https://github.com/mrcthms)! - Fixed display bug with `BulkActions` when content in the table changes at the same time the bulk actions bar is visible - [Shopify#7846](Shopify#7846) [`65131df18`](Shopify@65131df) Thanks [@qt314](https://github.com/qt314)! - - Changed border radius of `focus-ring` to use shape tokens instead of adding an extra `1px` - Updated `TextField` placeholder text to not change to a custom color on error - Delete an unused class in the `DataTable` and `IndexTable` - Replace some `px` values with tokens - Add some stylelint ignore context comments - [Shopify#7891](Shopify#7891) [`ff82bdb18`](Shopify@ff82bdb) Thanks [@mrcthms](https://github.com/mrcthms)! - Updated the colors of the borders of the buttons in the `BulkActions` component ## @shopify/plugin-polaris@0.0.21 ### Patch Changes - Updated dependencies \[]: - @shopify/polaris-migrator@0.10.1 ## @shopify/polaris-migrator@0.10.1 ### Patch Changes - Updated dependencies \[[`4b96147b3`](Shopify@4b96147), [`c8a294f51`](Shopify@c8a294f), [`6cd75fd9a`](Shopify@6cd75fd), [`bdf04a289`](Shopify@bdf04a2), [`9206b7b13`](Shopify@9206b7b)]: - @shopify/stylelint-polaris@5.0.1 ## @shopify/stylelint-polaris@5.0.1 ### Patch Changes - [Shopify#7866](Shopify#7866) [`4b96147b3`](Shopify@4b96147) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Updated `stylelint-polaris` to allow disabling specific coverage rules instead of entire categories - [Shopify#7906](Shopify#7906) [`c8a294f51`](Shopify@c8a294f) Thanks [@qt314](https://github.com/qt314)! - Fixed config atRule and function regex to match whole word - [Shopify#7868](Shopify#7868) [`6cd75fd9a`](Shopify@6cd75fd) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Update `package.json` to define a single entry point - [Shopify#7696](Shopify#7696) [`bdf04a289`](Shopify@bdf04a2) Thanks [@chloerice](https://github.com/chloerice)! - Implemented custom message configuration support for polaris/coverage plugin - [Shopify#7898](Shopify#7898) [`9206b7b13`](Shopify@9206b7b) Thanks [@qt314](https://github.com/qt314)! - Bug fix to wrap z-index 'declaration-property-value-allowed-list' token value with "var" ## polaris.shopify.com@0.27.2 ### Patch Changes - Updated dependencies \[[`53d836dc2`](Shopify@53d836d), [`9e617c4d9`](Shopify@9e617c4), [`5a4faf821`](Shopify@5a4faf8), [`65131df18`](Shopify@65131df), [`ff82bdb18`](Shopify@ff82bdb)]: - @shopify/polaris@10.16.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
WHY are these changes introduced?
Closes #7506
To do
Finish adding examples to coverage/README.md(handling in separate PR)WHAT is this pull request doing?
This pull request adds support for configuring
stylelint-polaris/coverage
rules with custom messages and metadata, so that error and warning messages tell admin builders how to resolve the problem and VS Code diagnostics link to the@shopify/stylelint-polaris
documentation.For example:
This PR also includes a bit of polish:
polaris/coverage
plugin so that primary options are clear in the config and in the plugin's codeHow to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes