-
Notifications
You must be signed in to change notification settings - Fork 268
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
Update eslint config to include JSX files #3139
Conversation
By default eslint only processes files with `.js` extension. Add an override to include `.jsx` Run `npm run lint:fix` to address to issues that can be automatically resolved. Fix a11y bug with Task component keyboard nav that this highlighted.
@@ -13,7 +13,7 @@ limitations under the License. | |||
module.exports = { | |||
env: { | |||
browser: true, | |||
es2021: true, | |||
es2022: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required for eslint to support public class fields without requiring babel
@@ -53,7 +56,7 @@ module.exports = { | |||
'formatjs/enforce-default-message': 'error', | |||
'formatjs/enforce-id': 'error', | |||
'formatjs/no-complex-selectors': 'error', | |||
'formatjs/no-literal-string-in-jsx': 'error', | |||
'formatjs/no-literal-string-in-jsx': 'off', // TODO: [AG] re-enable this after cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of these, mostly the hardcoded Tekton resource kinds so intentionally left as literals. May revisit this rule in future if it becomes annoying but temporarily disabling for now.
@@ -112,16 +122,6 @@ export /* istanbul ignore next */ class PipelineRunContainer extends Component { | |||
})); | |||
}; | |||
|
|||
getPipelineTask = ({ pipelineRun, selectedTaskId, taskRun }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class method expected to use this
. No need for it to be a class method since it's a pure function so move it outside the component and update references
className="tkn--task-link" | ||
tabIndex={0} // eslint-disable-line jsx-a11y/no-noninteractive-tabindex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This meant that the Task element wasn't reachable by keyboard, so users couldn't access the Task status, pod info, results, etc. by keyboard. Also couldn't expand other tasks in the tree by keyboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: briangleeson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
Discovered while reviewing #3108 that eslint wasn't processing our JSX files.
By default eslint only processes files with
.js
extension. Add an override to include.jsx
Run
npm run lint:fix
to address to issues that can be automatically resolved.Fix a11y bug with Task component keyboard nav that this highlighted.
/kind misc
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes