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.

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

Prepare 23.0.0 #198

Merged
merged 10 commits into from Oct 21, 2021
Merged

Prepare 23.0.0 #198

merged 10 commits into from Oct 21, 2021

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Sep 18, 2021

We're making changes to get users up and running quickly with comprehensive linting that doesn't leave gaps, e.g. by encouraging users to extend an appropriate shared-config for their choice of styling language.

This pull request is part of those changes. It turns on a handful of rules that'll help new users write modern CSS that looks like something they'd see in the specifications.

For context, our previous approach was to provide the minimal configurations and suggest users build on top of them, going as far as originally emphasising this config and the recommended one in our docs equally. However, I don't think this is helpful for the majority of new users, even those who intend to use stylelint and a pretty-printer like Prettier together, as there are a number of stylistic rules in stylelint and this plugin that are complementary to Prettier, e.g. the *-empty-line-before ones. Better to extend a standard config and use *-config-prettier to turn off conflicting rules.

Power users who wish to craft their own config on top of the recommended config can still do so. The changes in this pull request are to provide better linting for non-power users writing CSS.

Ref: stylelint/stylelint#5205

[Google's CSS Style Guide](https://google.github.io/styleguide/htmlcssguide.html#CSS_Formatting_Rules), [Airbnb's Styleguide](https://github.com/airbnb/css#css), and [@mdo's Code Guide](https://codeguide.co/#css).

It favours flexibility over strictness for things like multi-line lists and single-line rulesets, and tries to avoid potentially divisive rules.

Use it as is or as a foundation for your own config.
Copy link
Member Author

@jeddy3 jeddy3 Sep 18, 2021

Choose a reason for hiding this comment

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

We can take this second line out as we encourage users (lower down the README) to extend the config with more rules that limit language features, e.g. unit-allowed-list. These are some of the most useful, unique and powerful rules in stylelint. We should encourage users to investigate them. They tend to be specific to teams and projects, so we can't add them to this config.

@@ -100,23 +108,7 @@ npm install stylelint-config-standard --save-dev

## Usage

If you've installed `stylelint-config-standard` locally within your project, just set your `stylelint` config to:
Copy link
Member Author

Choose a reason for hiding this comment

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

We can take out this legacy guidance.


/**
* Multi-line comment
*/

:root {
--brand-red: hsl(5deg 10% 40%);
Copy link
Member Author

Choose a reason for hiding this comment

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

We can add an example of modern custom properties.

Comment on lines -41 to +43
top: calc(calc(1em * 2) / 3);
top: calc(100% - 2rem);
Copy link
Member Author

Choose a reason for hiding this comment

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

We can remove this odd nested calc.

Comment on lines -59 to +61
background: color(rgb(0, 0, 0) lightness(50%));
font-family: helvetica, "arial black", sans-serif;
background: hsl(20deg 25% 33%);
Copy link
Member Author

Choose a reason for hiding this comment

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

We can remove this old color function.

Incidentally, the upcoming relative color syntax looks great!

- Specify a maximum line length using:
- [`max-line-length`](https://github.com/stylelint/stylelint/blob/master/lib/rules/max-line-length/README.md)

### Using the config with SugarSS syntax
Copy link
Member Author

Choose a reason for hiding this comment

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

We can take this section out as we'll be encouraging users to adopt appropriate shared-configs for their language of choice.

index.js Outdated
@@ -37,13 +40,16 @@ module.exports = {
ignore: ['after-comment', 'inside-single-line-block'],
},
],
'custom-media-pattern': /^([a-z][a-z0-9]*)(-[a-z0-9]+)*$/,
Copy link
Member

Choose a reason for hiding this comment

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

I think all rules with RegExps would be confusing for non-power user. Because if there is a violation we will show this RegExp and use might not be familiar with it at all.

Copy link
Member Author

@jeddy3 jeddy3 Sep 18, 2021

Choose a reason for hiding this comment

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

Good point.

We can use the message secondary option to make them simpler, e.g.:

Expected custom media name to be kebab-case

It's one of the reasons we have that option.

Copy link
Member

Choose a reason for hiding this comment

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

We could try that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Each is slightly adapted from the original message in the rule.

@jeddy3 jeddy3 mentioned this pull request Sep 19, 2021
30 tasks
@ntwb
Copy link
Member

ntwb commented Sep 20, 2021

Love this, and whilst I am in broad agreement with everything proposed, I've one question:

https://www.npmjs.com/package/stylelint-config-standard has 1,337,936 weekly downloads, this is pretty significant (and pretty cool), but when this is released it will be a breaking change...

Would it be worth considering that for stylelint v14 these new rule changes are added as warnings, then in the next major release changed to the default error.

I'm thinking this might help facilitate non-blocking uptake of the new release for users, this then gives them time to update their CSS before the following major release?

@jeddy3
Copy link
Member Author

jeddy3 commented Sep 25, 2021

I'd prefer to have new rules as errors to be consistent with previous major releases on this package. However, I understand your point. What we can do is better emphasise in the README how to turn rules off or change their severity to warnings. I'll amend the pull request to do this. We'll then be consistent in our recommendation that users adopt the standard (rather than recommended) config and then turn off any rules they don't need, whether that's by adding a rules property or using a shared config like stylelint-config-prettier.

@jeddy3 jeddy3 changed the title Add rules to help new users get started Prepare 23.0.0 Sep 27, 2021
@jeddy3
Copy link
Member Author

jeddy3 commented Sep 27, 2021

@ntwb In 6db6439, I've expanded the "Extending the config" section of the README to give explicit examples of turning off and lowering the severity of rules. And I've linked to that section from the migration notes in the CHANGELOG.

Tests will pass once #199 is merged and this branch rebased.

This pull request is ready for review.

When stylelint 14 and recommended config 6 are released we should:

  1. Update the deps
  2. Merge
  3. Release

@ntwb
Copy link
Member

ntwb commented Sep 28, 2021

@ntwb In 6db6439 (#198), I've expanded the "Extending the config" section of the README to give explicit examples of turning off and lowering the severity of rules. And I've linked to that section from the migration notes in the CHANGELOG.

Thanks, looks great 💯

Tests will pass once #199 is merged and this branch rebased.

I approved and merged this in and updated the branch here in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants