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 warp ds eslint config #238

Merged
merged 14 commits into from
May 16, 2024
Merged

Conversation

felicia-haggqvist
Copy link
Contributor

@felicia-haggqvist felicia-haggqvist commented May 2, 2024

Fixes Jira issue: WARP-520

  • Add eslint configuration and workflow
  • lint all code using pnpm lint

@felicia-haggqvist felicia-haggqvist requested a review from a team May 2, 2024 09:05
@felicia-haggqvist felicia-haggqvist self-assigned this May 2, 2024
Copy link
Contributor

github-actions bot commented May 2, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-05-16 08:58 UTC

@felicia-haggqvist felicia-haggqvist marked this pull request as draft May 2, 2024 09:12
@felicia-haggqvist felicia-haggqvist marked this pull request as ready for review May 2, 2024 14:24
vite.config.js Outdated
"build.js",
"esbuild.js",
],
exclude: ['**.json', 'dev/**', 'storybook-static/**', '.storybook/**', 'packages/**/stories', 'packages/**/src/locales', 'esbuild.mjs', 'lingui.config.ts', '@types', 'tests/', 'packages/**/src/props.*', 'packages/**/src/index.*', 'packages/index.ts', 'build.js', 'esbuild.js'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't wrap my head around why this wrapped content was formated to be one line, while in other repos that didn't happen. We had the same setup e.g. in @warp-ds/vue, which extends @warp-ds/eslint-config. I see that in our Warp config we specified printWidth for prettier to 999 (code), which sounds like the setting responsible for this, but it only seems to apply in this repo. I'm also wondering, is printWidth: 999 rule for prettier something we would like to have? @warp-ds/warp-core-team

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't know why it didn't format the same way as for vue 🙈 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't exceed 140 "printWidth": 140,
This would work on most retina display macbooks without issues

Copy link
Contributor

Choose a reason for hiding this comment

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

It also might be because of "plugin:prettier/recommended" in the eslintrc 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it did that formatting before I added "plugin:prettier/recommended" in the eslintrc

Copy link
Contributor Author

@felicia-haggqvist felicia-haggqvist May 3, 2024

Choose a reason for hiding this comment

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

Although it looks like it had formatted the exclude array the same way in the vite.config.jsfile for vue. 🤔 See here

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it the "printWidth": 999 from @warp-ds/eslint-config that is kicking in here (and then it didn't in the other repos?)? When I added printWidth: 150 as a separate rule in .eslintrc here, it worked fine. 999 characters is an eternity in the world of an IDE, so I'm not sure why it was set to that in the first place 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it reacted the same way in vue as in react (see my previous comment). So it did kick in. Talked to @imprashast and we talked about that I should update the @warp-ds/eslint-config to have printWidth: 140 :)

@felicia-haggqvist felicia-haggqvist marked this pull request as draft May 3, 2024 10:21
@felicia-haggqvist felicia-haggqvist marked this pull request as ready for review May 15, 2024 15:12
Copy link
Contributor

@BalbinaK BalbinaK left a comment

Choose a reason for hiding this comment

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

Just one comment and we should be good to go!

- name: Install pnpm and dependencies
uses: pnpm/action-setup@v2
with:
version: 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version: 8
version: 9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch!

@felicia-haggqvist felicia-haggqvist merged commit d83f799 into next May 16, 2024
4 checks passed
@felicia-haggqvist felicia-haggqvist deleted the feat/add-warp-ds-eslint-config branch May 16, 2024 08:57
github-actions bot pushed a commit that referenced this pull request May 16, 2024
# [1.6.0-next.1](v1.5.0...v1.6.0-next.1) (2024-05-16)

### Features

* add warp ds eslint config ([#238](#238)) ([d83f799](d83f799))
github-actions bot pushed a commit that referenced this pull request Jul 3, 2024
# [1.6.0](v1.5.0...v1.6.0) (2024-07-03)

### Bug Fixes

* add default targetEl for callout ([#248](#248)) ([e297880](e297880))
* bump core to fix slider ([#250](#250)) ([5192147](5192147))
* **slider:** Prevent onChange/onChangeAfter called on mount ([#253](#253)) ([84ddd64](84ddd64))

### Features

* add warp ds eslint config ([#238](#238)) ([d83f799](d83f799))
* slider onChangeAfter prop ([#247](#247)) ([fa1af5c](fa1af5c))
* **toggle:** add ReactNode type to toggle labels ([#244](#244)) ([abeff99](abeff99))
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.

3 participants