Skip to content
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

[EuiBasicTable] Remove preventing all mouse/keyboard interactions when loading #6543

Merged
merged 4 commits into from
Jan 24, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jan 24, 2023

Summary

closes #5014

While converting EuiBasicTable to Emotion, I happened upon this tbody logic/ref added when EuiInMemoryTable was first introduced (#390), although unfortunately, I could not find any context as to why it was introduced/approved.

Unfortunately, I strongly feel that this behavior is problematic at worst (accessibility - keyboard users who tab into a loading table will never be able to tab off) and totally unnecessary at best (we're adding and removing 11 event listeners on loading change, for very little UX benefit).

I'm proposing we remove this, with designer approval and approval of our resident a11y expert.

QA

General checklist

  • Revert [REVERT ME] commit
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6543/

@cee-chen cee-chen marked this pull request as ready for review January 24, 2023 17:17
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM! Tested for keyboard interaction and verified no regressions using Safari and Firefox with VoiceOver screen reader. Latest Chrome axe-core plugin returned 0 violations.

@cee-chen cee-chen enabled auto-merge (squash) January 24, 2023 18:53
@cee-chen cee-chen enabled auto-merge (squash) January 24, 2023 18:54
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6543/

@cee-chen cee-chen merged commit dc3fcbc into elastic:main Jan 24, 2023
@cee-chen cee-chen deleted the basic-table-loading branch January 24, 2023 19:31
jbudz pushed a commit to elastic/kibana that referenced this pull request Jan 27, 2023
## Summary

`eui@73.0.0` ⏩ `eui@74.0.1`

---

## [`74.0.1`](https://github.com/elastic/eui/tree/v74.0.1)

**Bug fixes**

- Fixed `EuiModalHeaderTitle` type errors when passed `EuiTitle` props
([#6547](elastic/eui#6547))

## [`74.0.0`](https://github.com/elastic/eui/tree/v74.0.0)

- Added the `component` prop to `EuiModalHeaderTitle`, which allows
overriding the default `h1` tag
([#6530](elastic/eui#6530))
- Added the `titleProps` prop to `EuiConfirmModal`, which allows
overriding the default `h1` tag
([#6530](elastic/eui#6530))

**Bug fixes**

- Fixed slight row height jumping in `EuiBasicTable`s when actions with
tooltips became disabled
([#6538](elastic/eui#6538))

**Breaking changes**

- `EuiModalHeaderTitle` now **always** wraps its children in a `h1` tag
(previously attempted to conditionally detect whether its children were
raw strings or not). To change this tag type to, e.g. a more generic
`div`, use the new `component` prop.
([#6530](elastic/eui#6530))
- `EuiLink` now applies `rel="noreferrer"` to all domains, including
`elastic.co` ([#6535](elastic/eui#6535))
- `EuiBasicTable` no longer blocks mouse/keyboard interactions while
`loading` ([#6543](elastic/eui#6543))

**CSS-in-JS conversions**

- Converted `EuiBasicTable` to Emotion
([#6539](elastic/eui#6539))
- Added a new `RenderWithEuiTheme` render prop utility
([#6539](elastic/eui#6539))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023
## Summary

`eui@73.0.0` ⏩ `eui@74.0.1`

---

## [`74.0.1`](https://github.com/elastic/eui/tree/v74.0.1)

**Bug fixes**

- Fixed `EuiModalHeaderTitle` type errors when passed `EuiTitle` props
([elastic#6547](elastic/eui#6547))

## [`74.0.0`](https://github.com/elastic/eui/tree/v74.0.0)

- Added the `component` prop to `EuiModalHeaderTitle`, which allows
overriding the default `h1` tag
([elastic#6530](elastic/eui#6530))
- Added the `titleProps` prop to `EuiConfirmModal`, which allows
overriding the default `h1` tag
([elastic#6530](elastic/eui#6530))

**Bug fixes**

- Fixed slight row height jumping in `EuiBasicTable`s when actions with
tooltips became disabled
([elastic#6538](elastic/eui#6538))

**Breaking changes**

- `EuiModalHeaderTitle` now **always** wraps its children in a `h1` tag
(previously attempted to conditionally detect whether its children were
raw strings or not). To change this tag type to, e.g. a more generic
`div`, use the new `component` prop.
([elastic#6530](elastic/eui#6530))
- `EuiLink` now applies `rel="noreferrer"` to all domains, including
`elastic.co` ([elastic#6535](elastic/eui#6535))
- `EuiBasicTable` no longer blocks mouse/keyboard interactions while
`loading` ([elastic#6543](elastic/eui#6543))

**CSS-in-JS conversions**

- Converted `EuiBasicTable` to Emotion
([elastic#6539](elastic/eui#6539))
- Added a new `RenderWithEuiTheme` render prop utility
([elastic#6539](elastic/eui#6539))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EuiBasicTable blocks any action while isLoading is true
3 participants