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

[A11y][TableListView] Refactor to use context to return focus #188774

Merged
merged 5 commits into from
Jul 23, 2024

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Jul 19, 2024

Summary

Closes https://github.com/elastic/observability-dev/issues/3345

Refactoring the tags portion of the table list view to return focus after closing the tags popover

@rshen91
Copy link
Contributor Author

rshen91 commented Jul 19, 2024

/ci

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 19, 2024

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #100 / saved objects tagging - functional tests visualize integration listing allows to filter by multiple tags
  • [job] [logs] FTR Configs #100 / saved objects tagging - functional tests visualize integration listing allows to filter by multiple tags
  • [job] [logs] Jest Tests #17 / TableListView tag filtering should filter by tag from the search bar filter
  • [job] [logs] Jest Tests #17 / TableListView tag filtering should filter by tag from the search bar filter

History

cc @rshen91

@rshen91 rshen91 added the accessibility: focus Accessibility focus management label Jul 19, 2024
@sebelga
Copy link
Contributor

sebelga commented Jul 22, 2024

/ci

@@ -43,7 +43,8 @@ const saveBtnWrapperCSS = css`
width: 100%;
`;

interface Props {
interface Context {
enabled: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to pass the enabled and handle it on L61 as the consumer (the panel) does not render if tagging is not enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - commit ed518a2 for changes

@rshen91 rshen91 marked this pull request as ready for review July 22, 2024 15:56
@rshen91 rshen91 requested a review from a team as a code owner July 22, 2024 15:56
@rshen91 rshen91 added the release_note:skip Skip the PR/issue when compiling release notes label Jul 22, 2024
@rshen91 rshen91 requested a review from sebelga July 22, 2024 19:49
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 533.2KB 533.4KB +278.0B
eventAnnotationListing 302.8KB 303.1KB +279.0B
filesManagement 111.3KB 111.6KB +279.0B
graph 403.9KB 404.2KB +279.0B
maps 3.0MB 3.0MB +279.0B
visualizations 292.8KB 293.1KB +279.0B
total +1.6KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @rshen91

@rshen91 rshen91 enabled auto-merge (squash) July 22, 2024 22:16
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this @rshen91!

@rshen91 rshen91 merged commit c11d534 into elastic:main Jul 23, 2024
21 checks passed
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Jul 23, 2024
/>
<TagFilterContextProvider
isPopoverOpen={isPopoverOpen}
isInUse={isInUse}
Copy link
Contributor

Choose a reason for hiding this comment

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

@sebelga, if I am not mistaken, we can simplify further because isInUse was a workaround for the problem that should be solved with context

Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be checked but it could probably be removed indeed 👍

@rshen91 rshen91 deleted the table-list-view-focus-tab branch July 23, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility: focus Accessibility focus management backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants