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

feat: add stylelint #464

Merged
merged 14 commits into from
May 15, 2024
Merged

feat: add stylelint #464

merged 14 commits into from
May 15, 2024

Conversation

ismay
Copy link
Member

@ismay ismay commented Apr 17, 2024

https://dhis2.atlassian.net/browse/DHIS2-16776

  • Add stylelint config (d2 style add stylelint)
  • Remove stylelint config (d2 style remove stylelint)
  • Check css
    • d2 style check css
    • d2 style check css <filename>
    • d2 style check css --staged
  • Autofixing css
    • d2 style apply css (applies prettier and stylelint fixes)
  • Update readme

Note:

Unrelated, but for some reason the dotfiles at the root of this repo were executable. There's no need for that, so I removed it: 160c29a

Follow up:

  • Add other rules for stylelint (like their base config, etc.)
  • Add other types (like css-in-js, etc.)
  • Ignorefile handling

@ismay ismay marked this pull request as ready for review May 7, 2024 12:21
@ismay ismay requested a review from a team May 7, 2024 12:26
@ismay
Copy link
Member Author

ismay commented May 10, 2024

In thinking about this, it might be better to just make this an error right away. I think all these warnings can be autofixed (we should doublecheck that). But we could do that later as well.

Copy link
Contributor

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

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

Tool works as expected, just one minor thing:
I just used this to add stylelint to a local dhis2 repository. When trying to check for stylelint issues, I'm getting the following warning:

../cli-style/bin/d2-style check css
css > stylelint 
Error: No rules found within configuration. Have you provided a "rules" property?
    at configurationError (file:///home/gerkules/development/dhis2/cli-style/node_modules/stylelint/lib/utils/configurationError.mjs:12:49)

When I add rules: [] to the .stylelintrc.js, the warning is gone.

@@ -58,7 +58,7 @@ jobs:
token: ${{secrets.DHIS2_BOT_GITHUB_TOKEN}}
- uses: actions/setup-node@v2
with:
node-version: 12.x
node-version: 20.x
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this need to install npm@6, just like the UI lib?

Copy link
Member Author

Choose a reason for hiding this comment

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

As long as we don't have the peerdependency issues that ui has we shouldn't. I don't think this lib has that problem, so we should be good.

@ismay
Copy link
Member Author

ismay commented May 13, 2024

Tool works as expected, just one minor thing: I just used this to add stylelint to a local dhis2 repository. When trying to check for stylelint issues, I'm getting the following warning:

../cli-style/bin/d2-style check css
css > stylelint 
Error: No rules found within configuration. Have you provided a "rules" property?
    at configurationError (file:///home/gerkules/development/dhis2/cli-style/node_modules/stylelint/lib/utils/configurationError.mjs:12:49)

When I add rules: [] to the .stylelintrc.js, the warning is gone.

Weird, the config that this tool installs has rules, see here. It's referenced via the template that we install, here. Did you install stylelint with d2 style add stylelint? If so, does the .stylelintrc.js in the root of your project look like the template?

@ismay
Copy link
Member Author

ismay commented May 14, 2024

Follow up to the previous comment: I've had a horrible time on my desktop trying to get yarn link to work with our monorepo for @dhis2/cli. Eventually I gave up and went back to macos. There I could confirm that d2 style add stylelint adds what I quoted above:

const { config } = require('@dhis2/cli-style')

module.exports = {
    extends: [config.stylelint],
}

So it should be good. Let me know.

Copy link
Contributor

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

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

I didn't get these issues when I actually linked the library, my bad!
There's one inconsistency: formatting the css lint issues doesn't print the files that were touched during that process, unlike what the other tools do.
Not really a problem IMO, so I approve

@ismay
Copy link
Member Author

ismay commented May 15, 2024

I double checked, it seems like only prettier outputs the files that it formatted. I've looked at whether eslint supports outputting the files that it touched with --fix, but it doesn't seem like it does. So we're limited by what the tools themselves support.

@ismay ismay merged commit 4a8c70b into master May 15, 2024
13 checks passed
@ismay ismay deleted the add-stylelint branch May 15, 2024 11:12
dhis2-bot added a commit that referenced this pull request May 15, 2024
# [10.6.0](v10.5.2...v10.6.0) (2024-05-15)

### Features

* add stylelint ([#464](#464)) ([4a8c70b](4a8c70b))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 10.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants