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

Enforce order for mixin #17

Merged
merged 1 commit into from
Apr 3, 2022
Merged

Conversation

PaarthAgarwal
Copy link
Member

I have enforced mixins as first in declarations. Fixes #14

@PaarthAgarwal
Copy link
Member Author

I really need to learn how to use prettier😅

@lb-
Copy link
Member

lb- commented Mar 17, 2022

@PaarthAgarwal what editor do you use? happy to help you get your prettier set up.

As for this PR - can you please add a 'passing' example to the valid.scss https://github.com/wagtail/stylelint-config-wagtail/blob/main/__tests__/scss-valid.scss

and a failing example to the invalid scss https://github.com/wagtail/stylelint-config-wagtail/blob/main/__tests__/scss-invalid.scss

We are not currently too strict on unit tests in this repo but in this case it is an easy thing to add and also ensures there is a visual reference in the PR of what we are blocking/allowing.

@PaarthAgarwal
Copy link
Member Author

@PaarthAgarwal what editor do you use? happy to help you get your prettier set up.

I appreciate your help. Currently I am using VS Code

As for this PR - can you please add a 'passing' example to the valid.scss https://github.com/wagtail/stylelint-config-wagtail/blob/main/__tests__/scss-valid.scss

and a failing example to the invalid scss https://github.com/wagtail/stylelint-config-wagtail/blob/main/__tests__/scss-invalid.scss

We are not currently too strict on unit tests in this repo but in this case it is an easy thing to add and also ensures there is a visual reference in the PR of what we are blocking/allowing.

Sure I'll add them right now

@PaarthAgarwal
Copy link
Member Author

@PaarthAgarwal what editor do you use? happy to help you get your prettier set up.

As for this PR - can you please add a 'passing' example to the valid.scss https://github.com/wagtail/stylelint-config-wagtail/blob/main/__tests__/scss-valid.scss

Since we just enforced order of mixins I have moved @mixin to top in valid.css.

and a failing example to the invalid scss https://github.com/wagtail/stylelint-config-wagtail/blob/main/__tests__/scss-invalid.scss

Here @mixin was already not at the top so I feel this should be valid enough for the invalid.css to be invalid (or invalid enough for the invalid.css to be valid)

We are not currently too strict on unit tests in this repo but in this case it is an easy thing to add and also ensures there is a visual reference in the PR of what we are blocking/allowing.

Let me know if I should add separate code for them or this approach will work.

@PaarthAgarwal
Copy link
Member Author

Linting issues are fixed but some errors are coming because of valid.css. Tried a few fixes but stuck.
Need help

@lb-
Copy link
Member

lb- commented Apr 3, 2022

OK this was a bit tricky, the issue was that when the tests fail they fail inside a promise, the simplest way around this when working locally is via logging.

Once logging was added it looks like the usage of unspecified was incorrect, I have instead changed this to 'declarations' as this seems to cover the use case fine.

Finally, there was some confusion I think the requirement was actually for the usage of mixins (via include) not the declaration of mixins (via @mixin).

I have pushed to your branch and will merge in shortly.

Thanks for this @PaarthAgarwal

@lb- lb- merged commit f7b2349 into wagtail:main Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce declarations order with stylelint
2 participants