move react/jsx-a11y rules into shared react preset #49244

spalger · 2019-10-24T19:43:21Z

It was pointed out that the react lint rules were only applied to JS files when we moved TS linting over to ESLint. This has been the case for a long time, so we have a lot of violations.

I'm not 100% sure about what we should do here, many of these violations can be auto-fixed, but those autofixes include adding dependencies to hooks which don't just work out of the box without inspection, and might cause undesirable effects, so someone is going to need to carefully consider many of these fixes.

Full list of violations: https://gist.github.com/spalger/b3702d46b004f485c9bcea29f6a0e36c
A summary of violation count by area:

packages/kbn-ui-framework: 1
src/core/public/application: 1
src/core/public/rendering: 1
src/legacy/core_plugins/console: 11
src/legacy/core_plugins/data: 7
src/legacy/core_plugins/expressions: 1
src/legacy/core_plugins/kbn_doc_views: 2
src/legacy/core_plugins/kbn_vislib_vis_types: 14
src/legacy/core_plugins/kibana_react: 1
src/legacy/core_plugins/kibana: 6
src/legacy/core_plugins/tile_map: 1
src/legacy/core_plugins/vis_type_markdown: 1
src/legacy/core_plugins/vis_type_metric: 1
src/legacy/core_plugins/vis_type_table: 3
src/legacy/core_plugins/vis_type_vega: 2
src/legacy/ui/public/capabilities: 3
src/legacy/ui/public/vis: 44
src/plugins/es_ui_shared: 5
src/plugins/eui_utils: 1
src/plugins/kibana_react: 10
src/plugins/kibana_utils: 1
test/plugin_functional/plugins/core_plugin_a: 1
test/plugin_functional/plugins/search_explorer: 4
x-pack/legacy/plugins/apm: 8
x-pack/legacy/plugins/beats_management: 2
x-pack/legacy/plugins/canvas: 11
x-pack/legacy/plugins/code: 27
x-pack/legacy/plugins/cross_cluster_replication: 4
x-pack/legacy/plugins/graph: 6
x-pack/legacy/plugins/index_management: 9
x-pack/legacy/plugins/infra: 60
x-pack/legacy/plugins/lens: 32
x-pack/legacy/plugins/ml: 134
x-pack/legacy/plugins/monitoring: 2
x-pack/legacy/plugins/security: 3
x-pack/legacy/plugins/siem: 66
x-pack/legacy/plugins/snapshot_restore: 7
x-pack/legacy/plugins/spaces: 3
x-pack/legacy/plugins/transform: 16
x-pack/legacy/plugins/upgrade_assistant: 5
x-pack/legacy/plugins/uptime: 13
x-pack/legacy/plugins/watcher: 11
x-pack/plugins/reporting: 1

I'm not really sure what to do here so I'm pushing this up for ideas.

elasticmachine · 2019-10-24T20:56:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 8496d7e

walterra · 2019-10-25T06:12:05Z

One suggestion comes to mind: Could this PR set up the overall config but with overrides to disable the rules again in individual plugins? Then each team could follow up in their own PRs, fix the rules and remove the overrides eventually.

spalger · 2019-10-25T07:14:20Z

Yeah, I like that idea @walterra, I'll create issues for each plugin/area and maybe @timroes can help me get them assigned to the correct teams

timroes · 2019-10-25T08:01:15Z

@spalger I like that idea. Given that I assume a lot of the failures are from the a11y part, depending on how much time @myasonik currently has, we can also try to support other teams creating fixing PRs for the a11y breakages.

myasonik · 2019-10-25T09:18:20Z

I think I'm missing something. Why the focus on jsx-a11y here?

Adding up the numbers in the summary, it looks like there are 542 errors. Doing a find on the full list of violations for jsx-a11y/ brings back 12 results, 11 of which are the same error (Visible, non-interactive elements with click handlers must have at least one keyboard listener jsx-a11y/click-events-have-key-events). I'm betting a majority of those errors can be fixed by replacing whatever element is being used with a button and if not that, I'm happy to help out.

(The only other a11y error is "onBlur must be used instead of onchange..." which should be a quick fix.)

timroes · 2019-10-25T10:15:52Z

Okay awesome, then my assumption was wrong :D Even better, but having only a couple of a11y issues hanging around, do you think we could help fixing those across the different plugins?

myasonik · 2019-10-25T10:38:16Z

On one hand, yes, we/I could submit a PR fixing all of the a11y issues.

On the other hand, that doesn't solve the broader linting problem (so I don't see the hurry) and it shielded people from learning about/practicing keyboard accessibility in a pretty approachable way (linting pointing to a specific line with good messaging and more docs online).

From an a11y education standpoint, I'd certainly rather see the plugin maintainers fix the issues and jump into to help them if it's unclear or review PRs or however else they might need support.

…les-global

elasticmachine · 2019-10-25T20:59:16Z

Pinging @elastic/kibana-operations (Team:Operations)

…les-global

elasticmachine · 2019-10-29T05:10:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1547af5

streamich · 2019-10-29T08:31:23Z

53 errors come from react/self-closing-comp rule, which is just a styling choice and could be disabled.

walterra

ML changes LGTM.

streamich

App Architecture code changes LGTM.

weltenwort

infra changes LGTM

legrego

Spaces changes LGTM

spalger · 2019-10-29T13:15:27Z

I'm going to approve on behalf of operations and move on without Canvas so others can get working on removing their overrides.

* move react/jsx-a11y rules into shared react preset * autofix react/jsx-closing-tag-location * autofix react/no-unknown-property * manually fix react/no-unescaped-entities * maually fix react/jsx-pascal-case * manually fix react/prefer-stateless-function * disable known violations in specific plugins/areas * remove code override # Conflicts: # x-pack/legacy/plugins/upgrade_assistant/public/components/tabs/checkup/deprecations/reindex/flyout/warnings_step.tsx

spalger · 2019-10-29T14:47:47Z

7.x/7.6: 0ee219e

This removes the two eslint exceptions specific to the `infra` plugin introduced in #49244. fixes #49563

This removes the two eslint exceptions specific to the `infra` plugin introduced in elastic#49244. fixes elastic#49563

move react/jsx-a11y rules into shared react preset

8496d7e

spalger added the discuss label Oct 24, 2019

spalger added 7 commits October 25, 2019 09:11

Merge branch 'master' of github.com:elastic/kibana into make-react-ru…

61404ea

…les-global

Merge branch 'master' of github.com:elastic/kibana into make-react-ru…

bc4f8d9

…les-global

autofix react/jsx-closing-tag-location

330cd81

autofix react/no-unknown-property

d4db33b

manually fix react/no-unescaped-entities

8d9c706

maually fix react/jsx-pascal-case

acfc0ca

manually fix react/prefer-stateless-function

241b327

spalger mentioned this pull request Oct 25, 2019

[meta] Fix disabled React/a11y ESLint rules #49381

Open

31 tasks

spalger added 2 commits October 25, 2019 12:24

disable known violations in specific plugins/areas

699642d

Merge branch 'master' of github.com:elastic/kibana into make-react-ru…

297a1e8

…les-global

This comment has been minimized.

Sign in to view

spalger added the Team:Operations Team label for Operations Team label Oct 25, 2019

spalger added v7.6.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes and removed discuss labels Oct 25, 2019

spalger marked this pull request as ready for review October 28, 2019 19:25

spalger requested a review from a team as a code owner October 28, 2019 19:25

spalger requested a review from a team October 28, 2019 19:25

spalger requested a review from a team as a code owner October 28, 2019 19:25

spalger added 2 commits October 28, 2019 20:42

Merge branch 'master' of github.com:elastic/kibana into make-react-ru…

03db982

…les-global

remove code override

1547af5

streamich mentioned this pull request Oct 29, 2019

Fix eslint #49595

Merged

walterra approved these changes Oct 29, 2019

View reviewed changes

streamich approved these changes Oct 29, 2019

View reviewed changes

weltenwort approved these changes Oct 29, 2019

View reviewed changes

weltenwort mentioned this pull request Oct 29, 2019

[Infra + Logs UI] Remove and fix linting rule exceptions #49610

Closed

legrego self-requested a review October 29, 2019 11:35

legrego approved these changes Oct 29, 2019

View reviewed changes

legrego mentioned this pull request Oct 29, 2019

Spaces - fix react hook deps #49618

Merged

spalger merged commit 19ea92a into elastic:master Oct 29, 2019

spalger mentioned this pull request Oct 29, 2019

[7.x] move react/jsx-a11y rules into shared react preset (#49244) #49631

Merged

spalger deleted the make-react-rules-global branch October 29, 2019 14:47

patrykkopycinski mentioned this pull request Oct 30, 2019

[SIEM] Fix eslint errors #49713

Merged

weltenwort mentioned this pull request Nov 18, 2019

[Logs + Metrics UI] Remove eslint exceptions #50979

Merged

weltenwort added a commit that referenced this pull request Dec 11, 2019

[Logs + Metrics UI] Remove eslint exceptions (#50979)

0cd62ca

This removes the two eslint exceptions specific to the `infra` plugin introduced in #49244. fixes #49563

weltenwort added a commit to weltenwort/kibana that referenced this pull request Dec 12, 2019

[Logs + Metrics UI] Remove eslint exceptions (elastic#50979)

6d3e729

This removes the two eslint exceptions specific to the `infra` plugin introduced in elastic#49244. fixes elastic#49563

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move react/jsx-a11y rules into shared react preset #49244

move react/jsx-a11y rules into shared react preset #49244

spalger commented Oct 24, 2019

elasticmachine commented Oct 24, 2019

walterra commented Oct 25, 2019

spalger commented Oct 25, 2019 •

edited

Loading

timroes commented Oct 25, 2019

myasonik commented Oct 25, 2019

timroes commented Oct 25, 2019

myasonik commented Oct 25, 2019

This comment has been minimized.

elasticmachine commented Oct 25, 2019

elasticmachine commented Oct 29, 2019

streamich commented Oct 29, 2019

walterra left a comment

streamich left a comment

weltenwort left a comment

legrego left a comment

spalger commented Oct 29, 2019

spalger commented Oct 29, 2019

move react/jsx-a11y rules into shared react preset #49244

move react/jsx-a11y rules into shared react preset #49244

Conversation

spalger commented Oct 24, 2019

elasticmachine commented Oct 24, 2019

💔 Build Failed

walterra commented Oct 25, 2019

spalger commented Oct 25, 2019 • edited Loading

timroes commented Oct 25, 2019

myasonik commented Oct 25, 2019

timroes commented Oct 25, 2019

myasonik commented Oct 25, 2019

This comment has been minimized.

elasticmachine commented Oct 25, 2019

elasticmachine commented Oct 29, 2019

💚 Build Succeeded

streamich commented Oct 29, 2019

walterra left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

spalger commented Oct 29, 2019

spalger commented Oct 29, 2019

spalger commented Oct 25, 2019 •

edited

Loading