chore: fix lint issues #857
Conversation
(cherry picked from commit 908f43cd643cc83c5db87516a435344d0ffb0517)
(cherry picked from commit c938d077c620937a3f3a2d362e04f1c9de7b7633)
| @@ -184,6 +184,12 @@ | |||
| ] | |||
| }, | |||
| "overrides": [ | |||
| { | |||
| "files": ["packages/run-it/**/*.ts{,x}", "packages/api-explorer/**/*.ts{,x}", "packages/hackathon/**/*.ts{,x}"], | |||
There was a problem hiding this comment.
Since we do not have tree shaking yet, the linter is wrong and I am disabling this rule across api-explorer, run-it and hackathon.
There was a problem hiding this comment.
It's worth noting that tree-shaking is live in the core product so it's likely this can get fixed across-the-board (I'd imagine in a future PR)
There was a problem hiding this comment.
If "testing-library/prefer-screen-queries" is going to be disabled I think it'd be a good tech-debt ticket to log to eventually enable it (see inline comment specific to that).
The Object.assign for defaultProps should be replaced with the attrs function from Styled Components
| @@ -184,6 +184,12 @@ | |||
| ] | |||
| }, | |||
| "overrides": [ | |||
| { | |||
| "files": ["packages/run-it/**/*.ts{,x}", "packages/api-explorer/**/*.ts{,x}", "packages/hackathon/**/*.ts{,x}"], | |||
There was a problem hiding this comment.
It's worth noting that tree-shaking is live in the core product so it's likely this can get fixed across-the-board (I'd imagine in a future PR)
| export const MDHeading = Object.assign(styled(Heading)``, { | ||
| defaultProps: { | ||
| mb: 'xsmall', | ||
| pt: 'xsmall', | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Rather than leveraging defaultProps at use attrs
| export const MDHeading = Object.assign(styled(Heading)``, { | |
| defaultProps: { | |
| mb: 'xsmall', | |
| pt: 'xsmall', | |
| }, | |
| }) | |
| export const MDHeading = styled(Heading).attrs( | |
| ({ mb = 'xsmall', pt = 'xsmall' }) => ({ px, py })`` |
There was a problem hiding this comment.
Seems like I missed your review by a few seconds. I will put up a fix for this fix
| @@ -195,7 +201,8 @@ | |||
| "rules": { | |||
| "testing-library/render-result-naming-convention": "off", | |||
| "@typescript-eslint/no-empty-function": "off", | |||
| "@typescript-eslint/ban-ts-comment": "off" | |||
| "@typescript-eslint/ban-ts-comment": "off", | |||
| "testing-library/prefer-screen-queries": "off" | |||
There was a problem hiding this comment.
I believe there's an auto-fix for this so it might be worth running with eslint --fix and seeing how many tests can get auto-fixed to use screen.
We have found that screen really helps test readability and solves for some common bugs around async behavior. Read-up at: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-screen
There was a problem hiding this comment.
I agree with this but @looker/redux is being copied as is from HT so we don't want to make any changes to it in this repo as it would be hard to maintain. We are using screen everywhere else that we control
This comment has been minimized.
This comment has been minimized.
No description provided.