Add constraints to stylelint-polaris/coverage disable comments

[aaronccasanova]

Part of #7531

WHAT is this pull request doing?

This PR introduces the ability to capture stylelint-disable comments applied to our coverage rules and validate authors have provided context why they are opting out of the system.

Linting stylelint-disable comments, targeting coverage rules, was accomplished by extending the stylelint-polaris/coverage plugin to post process failures against Stylelint disabled ranges and ensuring comment descriptions are both present and prefixed with polaris:. By leveraging stylelint-polaris/coverage for linting comments, we have the ability to further extend the behavior to enhance our coverage insights (for example: requiring categories in description prefixes polaris-colors:, ability to disable failing coverage polaris-skip:, etc.)

Examples

.my-class {
  /* stylelint-disable -- polaris: These properties override default styles in <external library> */
  color: #000 !important;
  background: #fff !important;
  /* stylelint-enable */
}

/* stylelint-disable-next-line -- polaris: This animation provides a unique merchant experience */
@keyframes amaze {}

If a stylelint-disable comment targeting a coverage rule is NOT prefixed with polaris: an error is reported:

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

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>
  );
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated the component's README.md with documentation changes
• Tophatted documentation changes in the style guide

[dominikwilkowski]

This looks great! LGTM

[alex-page]

Love this. I do want to make sure we are aligned with the polaris: prefix for disabling Polaris rules. It could be p-ignore p-skip p-context lots of different things. The main thing is that it is searchable and the value we want for the long term.

Not against polaris but might be worthwhile discussing before shipping.

[sam-b-rose]

Great points. We could change it if needed in a future major and have overlap of multiply supported prefixes. So maybe we start with polaris: for simplicity then can introduce a convention for others later on.

Just some alternatives prefixes:

• yo:
• give-me-the-context:
• but-why:
• promise-i-need-this:

[aaronccasanova]

Yes, great callout! My main focus in this iteration was ensuring we can capture disabled coverage rules and validate some form of context was provided for tracking. I landed on polaris: to avoid redundancy as the alternatives (stylelint-polaris:, p-ignore, p-skip, etc.) are already communicated by the stylelint-disable comment. That said, I'm happy to make any changes to the required prefix 👍

[sam-b-rose]

🎩 'd and works brilliantly 👏 very nice strategy and implementation

[sam-b-rose]

I thought only targeting the disabledRanges.all would be an issue if a specific rule name was used; however, because of how we are creating the rules dynamically, Stylelint won't let you target it since it isn't present as a static rule in the config 🤯 brilliant

[aaronccasanova]

Thanks, leveraging stylelint-polaris/coverage worked out well! Because we categorize rules with custom names and check them manually, I was able to easily filter out disabled coverage rules from disabledRanges.all:

Notice the above result.stylelint.disabledWarnings includes the category that was disabled e.g. /motion. This can be leveraged in future iterations to require authors define each disabled category e.g.

/* stylelint-disable -- p-colors, p-motion: context */

Note: I started to explore this behavior, but felt the logic was becoming too complex for the initial implementation

[sophschneider]

LGTM! Just had a comment about more generic error messaging to guide the user to fix the error more easily.

Also we should probably start considering how this will affect the roll out strategy, if we are adding a bunch of disable comments with generic context comments to start with, we don't want developers to copy and paste the generic context. Its unfortunately happened a lot in Shopify/web 😬

[sophschneider]

Maybe we could generalize this message to educate the user when they don't add any comment at all like in this test case and have an example of what they should be doing, i.e.

Suggested change

	message: `Missing "polaris:" prefix in disable comment description`,
	message: `Incorrect disable comment for stylelint-polaris. The correct format is: "/* ${disableComment} -- polaris: {Context on why the rule is disabled} */"`,

Hopefully you could get the disableComment from the disabledRange object to print out either stylelint-disable-next-line or stylelint-disable.

Hopefully this makes sense!

[aaronccasanova]

@sophschneider and I sync'd offline and landed on the follow error message:

[sam-b-rose]

Just a thought, but I think even a small rewording would make it more actionable:

- Expected /* stylelint-disable -- polaris: Context comment for disable */
+ Expected /* stylelint-disable -- polaris: Reason for disabling */

or

- Expected /* stylelint-disable -- polaris: Context comment for disable */
+ Expected /* stylelint-disable -- polaris: Add a reason for disabling */

or

- Expected /* stylelint-disable -- polaris: Context comment for disable */
+ Expected /* stylelint-disable -- polaris: Provide a reason for context */

[aaronccasanova]

Thanks for the feedback! We wen't with option 1: 3b97ef0