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

Doesn't handle CSS Modules composition #302

Closed
jgthms opened this issue Oct 14, 2015 · 6 comments
Closed

Doesn't handle CSS Modules composition #302

jgthms opened this issue Oct 14, 2015 · 6 comments
Milestone

Comments

@jgthms
Copy link

jgthms commented Oct 14, 2015

In CSS Modules, you have compositions, which basically act like @extend in Sass. That's why they should come first in the list of properties.

But the property-sort-order rule doesn't handle that.

For example:

.title
  composes: heading
  background: $background
  box-shadow: 0 2px 5px $shadow
  color: $title
  font-size: 17px
  margin-bottom: 20px
  text-align: center

Will output:

  14:3  error  Expected `background`, found `composes`    property-sort-order
  15:3  error  Expected `box-shadow`, found `background`  property-sort-order
  16:3  error  Expected `color`, found `box-shadow`       property-sort-order
  17:3  error  Expected `composes`, found `color`         property-sort-order

It would be useful if composes was ignored when ordering alphabetically.

@DanPurdy
Copy link
Member

This may need to be another whitelist option where whitelisted properties are ignored in property sort orders, I think going any further into forcing certain elements to be first or last for the alphabetical sort order would open up a whole can of worms..

@jgthms
Copy link
Author

jgthms commented Oct 16, 2015

Yes absolutely.

I think just ignoring the whole declaration if the property is composes: is sufficient, considering it only breaks for the property-sort-order rule.

I guess it's about not adding it to the properties object here?

@DanPurdy
Copy link
Member

Thinking about it we actually have a list of all valid CSS properties in the linter already to check property spelling so it makes sense to only apply this to valid/default css properties and ignore everything else.

@DanPurdy DanPurdy added this to the 1.4.0 milestone Oct 18, 2015
@gajus
Copy link
Contributor

gajus commented Oct 28, 2015

I am getting the same error:

║ 37       │ 5        │ warning  │ Expected `background`, found `composes`                │ property-sort-order  ║

But I have added the "composes" property to the "order" declaration:

property-sort-order:
    - 1
    - order:
        - composes
        - background
no-misspelled-properties:
    - 1
    - extra-properties:
        - composes

Is there a reason it does not work?

@DanPurdy
Copy link
Member

I can't replicate your issue @gajus I'm afraid, #331 is currently waiting to be merged to improve the way this rule handles custom properties so it may resolve there. I would just check your yaml is valid as this may cause issues

If you find this happening again once 1.4.0 is out then please submit a separate issue as the original issue here will be closed.

@jgthms sorry this has taken so long to finalise and get done but should be moving forward now!

@gajus
Copy link
Contributor

gajus commented Nov 17, 2015

I cannot replicate it in the most recent version either.

Thank you

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

No branches or pull requests

3 participants