feat: add ESLint, improve types, improve a11y #647
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/shoelace/shoelace/GG8s6aX1qj2ZkQu1NkhfqruB34aF |
|
Turns out there's a breaking change that was just released today with the latest versions of ESLint and TypeScript ESLint. typescript-eslint/typescript-eslint#4444. I assume it will get fixed soon since it's a pretty serious issue so I'll hold off on fixing this build for now. The PR is still ready to review, just will need a dependency update before merging. |
|
Note for any reviewers who want to test these changes locally before the ESLint issue gets fixed, be sure to |
There was a problem hiding this comment.
Thank you for this PR. I've reviewed every file and I don't think it's far off. Most of the suggestions are personal opinion/style, a few are possible oversights/bugs, and others are me asking questions out of curiosity.
Overall, I'm really happy with these changes and I can't wait to get this merged!
I still need to pull the files and play with things in VS Code. I suspect additional ESLint tweaks will be made if I find it getting in the way constantly, so I won't pester you about those just yet.
A couple things I want to call out:
- Thanks for refactoring the drag utility. It's obvious you were pretty deep into the code base to notice that.
- The grouping option for the test runner is fantastic
I think there were some other awesome things I called out in the comments, but they're escaping me right now. 😅
Thanks again. Let me know if you have questions or need clarification. I'll avoid any major commits to next until this merges to make it easier for you. (I could use a day or two off anyways!)
Will the dependency problem you mentioned prevent a merge? How does that impact this PR if they don't fix that bug in the next day or two?
| @@ -4,7 +4,10 @@ import { pascalCase } from 'pascal-case'; | |||
|
|
|||
| const packageData = JSON.parse(fs.readFileSync('./package.json', 'utf8')); | |||
| const { name, description, version, author, homepage, license } = packageData; | |||
| const noDash = string => string.replace(/^\s?-/, '').trim(); | |||
There was a problem hiding this comment.
Can we allow this? For simple functions, the overall code is more readable without splitting it out into multiple lines.
There was a problem hiding this comment.
We can always add an eslint-disable-next-line comment in some cases. Should I go through and revert any one-liner functions back to arrow functions with disable comments?
There was a problem hiding this comment.
I apologize because this is largely coding style and opinion, but making this allowed — at least for short definitions like this one — will really help me out.
|
@claviska Thank you for the thorough review! I've replied to most of the comments, many with follow-up questions. Let me know what you'd like to see and I'll go through and re-do it! I'll hold off on another revision until we settle on the details. |
|
Can we get the best of both worlds without using ignore comments? If so, updating the ternarys are fine. If not, reverting would be better for the sake of consistency and less ignore comments to think about.
|
|
To enable this PR to be merged, I've pinned ESLint to |
|
Thanks for tackling this. Looks great! |
|
For the record |
|
Updated in 8194d62. |
fixes #643
This is a massive PR. I apologize. But I think it'll be well worth it!
I've gone through and implemented everything we've agreed upon in #643 in addition to best practices which I'd recommend. This has uncovered several accessibility bugs, several edge cases for unchecked values, and more. Even without fixing the actual issues which were uncovered, being able to touch almost every file in the package in one go has enabled more reusable utilities to be created, new configs for running individual unit tests, and more.
I believe the spirit of the linked issue is to improve the overall quality of the package and make it easier to write code, safer, more robust, etc. This PR accomplishes that!
All of the tests pass, in addition to every example in every doc page also still works as expected with no errors.
I've re-read my code many times and compared diffs and I believe I've ensured that this keeps or fixes existing behaviors. While the PR is huge, it should be able to be reviewed line-by-line to ensure that the before and after do the same thing. Only a few instances require validations across multiple files.