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

Rework linting config #49

Merged
merged 4 commits into from
May 20, 2024
Merged

Conversation

Willem-Jaap
Copy link
Contributor

@Willem-Jaap Willem-Jaap commented May 19, 2024

This PR reworks the eslint & prettier configuration

Changes

Removed yarn.lock, there is already a package-lock.json file. In my opinion we should stick to a single package lock file, in previous pr's it has been mentioned yarn is not used.

Removed babel-eslint from the devDependencies, it is unmaintained and replaced by the already present @babel/eslint-parser. (Read about it here: https://www.npmjs.com/package/babel-eslint), this migration was already done in the eslint config (#28).

Removed eslint-plugin-prettier from the devDependencies, the prettier and eslint teams recommend to use each tool separately. (https://prettier.io/docs/en/integrating-with-linters.html#eslint).
A linter should be used for linting, a formatter for formatting. It also wasn't reporting any prettier errors to eslint. This behaviour is implemented by extending eslint-config-prettier.

Removed eslint-plugin-next from the devDependencies, @next/eslint-plugin-next, the plugin that powers the config is already present. Because we use a custom eslint config we implement the rules ourselves by extending that core plugin.

Implemented the next recommended rules in the custom eslint config. This now correctly extends the next config and reports the correct errors.

image

I also removed some unused code reported by eslint.

package.json is not formatted according to the prettier rules, along with a lot of the files in the repo. I'd love to run prettier --write . but thats a maintainer decision 😅

@taylorotwell
Copy link
Member

Why is open removed from the Dropdown if it is used just a few lines later?

@taylorotwell taylorotwell marked this pull request as draft May 20, 2024 13:28
@Willem-Jaap
Copy link
Contributor Author

Willem-Jaap commented May 20, 2024

The open state on line 34 is not derived from this useState hook.
This is a render prop, also called the function as child pattern (https://reactpatterns.js.org/docs/function-as-child-component/)

Instead of rendering a component headless ui returns the state as property in a callback so you can render custom components yourself: https://headlessui.com/react/menu#using-render-props

@Willem-Jaap Willem-Jaap marked this pull request as ready for review May 20, 2024 13:45
@taylorotwell taylorotwell merged commit deceb68 into laravel:master May 20, 2024
2 checks passed
@taylorotwell
Copy link
Member

Thanks 👍

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.

2 participants