chore: Fix linting errors #3350
Conversation
| @@ -113,7 +114,7 @@ module.exports = { | |||
| { | |||
| files: ['*.js', 'build-tools/**'], | |||
| rules: { | |||
| '@typescript-eslint/no-var-requires': 'off', | |||
| '@typescript-eslint/no-require-imports': 'off', | |||
There was a problem hiding this comment.
This was deprecated: https://typescript-eslint.io/rules/no-var-requires/
.eslintrc.js
Outdated
| @@ -27,6 +27,7 @@ module.exports = { | |||
| 'jest', | |||
| ], | |||
| rules: { | |||
| '@typescript-eslint/no-unused-expressions': 'off', | |||
There was a problem hiding this comment.
We have a lot of usage of this in our codebase
expression && doSomething()
There was a problem hiding this comment.
maybe just configure https://eslint.org/docs/latest/rules/no-unused-expressions#allowshortcircuit instead so we don't lose the other parts of this rule?
(although personally I don't like expression && doSomething() and think plain if statements are much more readable, but happy to go with team consensus)
There was a problem hiding this comment.
Makes sense to only configure the allowShortCircuit, will update and yes for this one I'm just going with the current status quo in the repo and not opening any discussions if it should or shouldn't be like that 😅
| @@ -101,7 +101,7 @@ export default class App extends React.Component { | |||
| status: hasNextPage ? 'pending' : 'finished', | |||
| options: this.pageNumber === 0 ? items : this.state.options.concat(items), | |||
| }); | |||
| } catch (error) { | |||
| } catch { | |||
There was a problem hiding this comment.
Unused and since its part of our dev pages it is safe to remove.
| const source = fs.readFileSync(file, 'utf-8'); | ||
| transformSync(source, { babelrc: false, configFile: false, plugins: [...defaultPlugins, extractor] })?.code; |
There was a problem hiding this comment.
It's a bit unclear what the original code was doing. Any ideas?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3350 +/- ##
========================================
Coverage 96.47% 96.47%
========================================
Files 805 805
Lines 22975 22975
Branches 7936 7523 -413
========================================
Hits 22164 22164
Misses 804 804
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
michaeldowseza
requested review from
gethinwebster
and removed request for
a team
| @@ -27,6 +27,7 @@ jest.mock('@cloudscape-design/component-toolkit/internal', () => ({ | |||
|
|
|||
| const tableRole = 'table'; | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
There was a problem hiding this comment.
why not just define a type here instead?
Co-authored-by: Gethin Webster <[email protected]>
| @@ -9,7 +9,7 @@ import styles from '../../../lib/components/live-region/test-classes/styles.css. | |||
|
|
|||
| const renderLiveRegion = async (jsx: React.ReactElement) => { | |||
| const { container } = render(jsx); | |||
| await waitFor(() => expect(document.querySelector('[aria-live=polite]'))); | |||
| await waitFor(() => expect(document.querySelector('[aria-live]')).toBeTruthy()); | |||
There was a problem hiding this comment.
Removed polite here as we have tests that render an assertive live region.
https://github.com/cloudscape-design/components/blob/main/src/live-region/__tests__/live-region.test.tsx#L90
Description
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.