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

Add selector-pseudo-class-focus rule #3355

Closed
wants to merge 1 commit into from

Conversation

YozhikM
Copy link
Contributor

@YozhikM YozhikM commented Jun 3, 2018

Which issue, if any, is this issue related to?

Issue #3354

Is there anything in the PR that needs further explanation?

No.

@ai
Copy link
Member

ai commented Jun 3, 2018

This rule will be very good for a11y. And could show that Stylelint is not just a tool to check whitespaces but could also promote best practices.

@YozhikM YozhikM changed the title feat: Add selector-pseudo-class-focus rule Add selector-pseudo-class-focus rule Jun 3, 2018
@jeddy3
Copy link
Member

jeddy3 commented Jun 4, 2018

@YozhikM Thanks for starting this! The code is looking good.

As I suggested in #2650 (comment), I think this and #3357 should first live in an stylelint-a11y plugin (pack).

Once the pack is published, you can send an PR to update the list of plugins.

@ai
Copy link
Member

ai commented Jun 4, 2018

@jeddy3 what is your reason for moving it to a plugin?

Here are my thoughts:

  1. This rule should be enabled in ost of projects.
  2. It is fit all core plugins rules.
  3. RIght now Stylelint needs more rules, which are really helpful in promoting best practices. Most of CSS enthusiasts ignores Stylelint since they see it as a tool just for linting spaces. Not to really important things. If we will have more a11y plugins in core, it will change their minds (plugin will not solve the problem, since nobody looks in plugins ;) )

@alexander-akait
Copy link
Member

@jeddy3 i agree with @ai

@hudochenkov
Copy link
Member

I would agree with @ai on this point. I would include this rule in the core, but won't include another one. Because in the current state it's for iPhone/iPad users. And very few people know about this feature.

@alexander-akait
Copy link
Member

@hudochenkov webkit also support this (i.e. chrome also and chromium based)

@jeddy3
Copy link
Member

jeddy3 commented Jun 4, 2018

what is your reason for moving it to a plugin?

I'm going to have to dig into my memory and recall some original design decisions, so bear with me...

When we first starting working on stylelint we looked to CSSLint for inspiration. We noticed that quite a few of the rules were now redundant as they were based on "best practises" that were, well, no longer best practice. As such, we made an early design decision to avoid adding such rules to stylelint, instead, we'd favour rules that could be configured to enforce whatever a team needs (or deems the best practice at the time) by applying limits to targeted constructs within the language.

That's why we have rules like declaration-property-value-blacklist instead of rules like transition-no-all and disallow-outline-none (from CSSLint).

This design decision is captured in the VISION doc.

A little later we realised that not everything a team might need could be achieved using just these configurable rules, so we added a plugin system. This allowed users to create rules which could do more than just apply limits to targeted constructs, whilst keeping the core stable and relevant.

In this particular instance, it looks like there are two ways to maintain accessibility:

  • specifying :focus and :hover together
  • relying on the browser default :focus behavior and not overiding it

I don't believe this rule can account for the latter, which is why it might not meet the "Has a clear and unambiguous finished state" rule of inclusion. Anyway, I think this point is moot as this is the kind of stuff that can get experimented on (and quickly iterated on) within a plugin.

Most of CSS enthusiasts ignores Stylelint since they see it as a tool just for linting spaces

It sounds like we need to do a better job of promoting the possible errors and limiting rules. I'm writing an article at the moment that highlights the value of the limiting rules. When I get around to finishing it it will hopefully help us promote this feature of stylelint.

(plugin will not solve the problem, since nobody looks in plugins ;) )

There are some excellent plugins that I see in regular use. stylelint-scss, stylelint-order and stylelint-csstree-validator and stylelint-selector-bem-pattern all spring to mind.

maybe it is normal put this rules in core, if somebody starts working on a11y plugin, we can move this out of stylelint in 10 version

We went through the pain of doing it that way around circa 7.8.0. I thought the lesson we learnt was that it's less painful to move plugins to core than to move built-in rules out.


I'm not seeing any arguments in the comments above that invalidate the original design decisions. Unless I'm missing something here? I agree there is a user education issue, though. We need to do a better job at promoting and signposting (within the docs and outside of them) the possible errors and limiting rules, as well as the plugin system (and this ally plugin).

An a11y stylelint plugin pack is an exciting proposition! Perhaps @YozhikM would like to create and champion it. Within the stylelint org itself, we can help out by promoting it.

It feels like this is approach will balance the user's need with the still valid original design decisions that have kept the built-in rules relevant. Thoughts?

@YozhikM
Copy link
Contributor Author

YozhikM commented Jun 4, 2018

@jeddy3 Excellent. Work in progress. Who and where can I ask questions if they appear?

@hudochenkov
Copy link
Member

@YozhikM thank you for starting working on this plugin! I think you can create an issue in this repository and we'll help you.

@jeddy3
Copy link
Member

jeddy3 commented Jun 4, 2018

Work in progress

Good stuff!

Both stylelint-scss and stylelint-order are plugin packs. You can use them for inspiration if you like.

@ai
Copy link
Member

ai commented Jun 5, 2018

There are some excellent plugins that I see in regular use. stylelint-scss, stylelint-order and stylelint-csstree-validator and stylelint-selector-bem-pattern all spring to mind.

And only 1/5 of Stylelint users use these plugins http://www.npmtrends.com/stylelint-vs-stylelint-order-vs-stylelint-scss

@alexander-akait
Copy link
Member

I still think it will be great as official plugin

@hudochenkov
Copy link
Member

And only 1/5 of Stylelint users use these plugins

Not everyone need these plugins :)

@ai
Copy link
Member

ai commented Jun 6, 2018

I am making slides about PostCSS and want to promote Stylelint. Here is an overview of our “Possible errors” rules:

  1. Prevent unknown props/selectors/colors, empty values, and wrong syntax
  2. Prevent duplicate property
  3. Prevent margin after margin-top
  4. Prevent descending specificity in selectors

Only 2 and 3 issues are really popular. Error 1 happens sometimes.

Unfortunately, it doesn’t compete with ESLint “Possible Errors” rules.

This is why I still believe that a11y rules in the core will be extremely good for promotion. We need more rules to prevent errors.

@ntwb
Copy link
Member

ntwb commented Jun 6, 2018

Unfortunately, it doesn’t compete with ESLint “Possible Errors” rules.

It is close, ESLint has 34 , stylelint has 24

https://stylelint.io/user-guide/rules/#possible-errors

@hudochenkov
Copy link
Member

Unfortunately, it doesn’t compete with ESLint “Possible Errors” rules.

Also, JavaScript is a programming language, while CSS isn't. More errors could be identified by code analysis in JavaScript, then in CSS.

@alexander-akait
Copy link
Member

I am glad to see more possible errors rules and i have some ideas for new rules, but i think we should concentrate now on bug fixes and build-in support html/markdown/css-in-js

@hudochenkov
Copy link
Member

@evilebottnawi I would like to see more error targeted rules, rather stylistic, as well (we might cover all stylistic rules already :)).

Please, create new issues for your rules ideas. New contributors are more willingly work on new stuff rather fixing bugs, in my opinion.

@alexander-akait
Copy link
Member

@hudochenkov In near future 👍

@ai
Copy link
Member

ai commented Jun 6, 2018

@ntwb when we compare ESLint and Stylelint rules by the number we are making mistakes. Stylelint has 10 rules just to check syntax errors (unknown properties, selectors, etc). If you group rules by error type we will have only 4 errors, which STylelint prevents

@hudochenkov we have JS in Stylelint to be smart. Simpler CSS syntax should even help us in finding real errors.

@jeddy3
Copy link
Member

jeddy3 commented Jun 6, 2018

I am making slides about PostCSS and want to promote Stylelint.

That's fantastic!

I'd suggest highlighting the limiting rules rather than focusing on the ones that check for possible errors. As @hudochenkov pointed out, CSS and JavaScript are two very different languages. They have different problems that need addressing with linters.

For me personally, I find that inconsistency is the biggest hurdle to maintaining a stylesheet within a team. There are so many ways for a developer to achieve the same outcome in CSS. As such, a large proportion of stylelint rules were specifically built to help teams combat these inconsistencies.

For example, a team might:

  • embrace a "mobile-first" approach and so use "media-feature-name-whitelist": ["min-width"] to ensure only min-width, rather than max-width, media queries are used
  • want to build a fluid site that proportionally scales and so use unit-whitelist: ["%", "em", "rem"] to allow only proportional units
  • only use BEM and so use selector-no-type: true, selector-no-universal: true etc to make sure only class selectors are used

These are just three examples. Hopefully, it's clear how stylelint can help a team enforce the consistent use of any number of other methodologies.

There are over 50 limiting rules in stylelint, including fine-grained ones like declaration-property-value-blacklist and the upcoming media-feature-name-value-whitelist. I think the limiting rules are worth highlighting as I believe:

  1. they give teams the power to solve the problem of inconsistency
  2. these rules are pretty unique to stylelint

I think we can close these PRs and issues now that #3364 has been merged.

We can continue the discussion about how best to promote the plugin system (and plugins themselves) in #3362.

@YozhikM Thanks again for your work on these rules and porting them to the a11y plugin! Feel free to jump into the #3362 discussion if you have any suggestions about how we can better promote your plugin and the plugin system itself.

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

6 participants