maximkoretskiy / postcss-autoreset

PostCSS plugin for automatic rules isolation
MIT License
244 stars 11 forks source link

Improve accuracy of SUIT CSS regex #17

Closed simonsmith closed 7 years ago

simonsmith commented 7 years ago

This adds some extra test cases for SUIT selectors and makes the regex more robust against edge cases.

^\.                  - Match a dot at the start of the string
(?:[a-z0-9]*-)?      - Then look for optional namespace
[A-Z]                - Match a single capital letter at the start of Component name
(?:[a-zA-Z0-9]+)     - Match the rest of the component name...
(?:-[a-zA-Z0-9]+)?$  - Match optional descendant

https://regex101.com/r/AA4xaq/3

Fixes #16

cc @giuseppeg

maximkoretskiy commented 7 years ago

I think Everything is OK. @giuseppeg Could U review this PR before I merge it?

simonsmith commented 7 years ago

to prematurely return if the css doesn't contain any @define directive i.e. there is no Component to reset.

Do you think that's needed here? Would be a breaking change. You can use SUIT and BEM without bem-linter.

giuseppeg commented 7 years ago

@simonsmith do you mean without having to add a @define directive? I always add it anyway but yeah if it is a breaking change better not do it.

Unless we see this fix as a breaking change too because in a way it'll break the current behaviour

simonsmith commented 7 years ago

True, it could be seen as breaking but if it's fixing a bug I would say it was just a patch.

Regardless though, I didn't think this plugin should enforce usage of @define. The main reason for that to exist is to derive the component name, which may not be needed in the future anyway and this plugin doesn't really care what your component name is, only that it exists.

I also wonder if users of BEM in particular use bem-linter as much as SUIT.

giuseppeg commented 7 years ago
I didn't think this plugin should enforce usage of `@define`

Agreed.

Up to you to decide whether to add the extra check before testing against the regexp – I am for add it. Either ways the branch is good to merge imo.

simonsmith commented 7 years ago

I see the logic in avoiding the regex 👍

Will add it

simonsmith commented 7 years ago

Okay, updated.

Please have a final look @maximkoretskiy @giuseppeg :)

giuseppeg commented 7 years ago

🚢 🇮🇹

awesome work! thank you @simonsmith

maximkoretskiy commented 7 years ago

Great!