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

fix(explore): skip Tab focus on control headers #16418

Closed
wants to merge 8 commits into from

Conversation

MaxHuiYYDS
Copy link
Contributor

@MaxHuiYYDS MaxHuiYYDS commented Aug 24, 2021

SUMMARY

Explore control panel:
I added hideNativeTab prop at ControlHeader in selectControl, when the native keyboard Tab actions need to omit the headers, then define the hideNativeTab to true.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

AFTER:

Screen.Recording.2021-08-22.at.8.40.08.PM.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Fixes Keyboard navigation in Explore control panel #14574
  • [] Required feature flags:
  • Changes UI
  • [] Includes DB Migration (follow approval process in SIP-59)
    • [] Migration is atomic, supports rollback & is backwards-compatible
    • [] Confirm DB migration upgrade and downgrade tested
    • [] Runtime estimates and downtime expectations provided
  • [] Introduces new feature or API
  • [] Removes existing feature or API

@MaxHuiYYDS
Copy link
Contributor Author

@junlincc @ktmud please help review, thanks.

@junlincc junlincc changed the title Fix(Explore control panel) native keyboard tab Fix(Explore control panel): native keyboard tab Aug 24, 2021
@junlincc
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@junlincc Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

@junlincc Ephemeral environment creation failed. Please check the Actions logs for details.

@ktmud ktmud changed the title Fix(Explore control panel): native keyboard tab fix(explore): skip Tab focus on control headers Aug 24, 2021
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #16418 (5d40847) into master (486ef6b) will increase coverage by 0.14%.
The diff coverage is 77.29%.

❗ Current head 5d40847 differs from pull request most recent head b2fd637. Consider uploading reports for the commit b2fd637 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16418      +/-   ##
==========================================
+ Coverage   76.55%   76.70%   +0.14%     
==========================================
  Files        1000     1002       +2     
  Lines       53487    53683     +196     
  Branches     6818     6855      +37     
==========================================
+ Hits        40948    41176     +228     
+ Misses      12301    12270      -31     
+ Partials      238      237       -1     
Flag Coverage Δ
javascript 71.01% <75.00%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...set-frontend/src/components/ModalTrigger/index.jsx 95.65% <ø> (ø)
...-frontend/src/components/OmniContainer/Omnibar.tsx 100.00% <ø> (ø)
...et-frontend/src/components/TableView/TableView.tsx 94.52% <ø> (ø)
superset-frontend/src/dashboard/actions/hydrate.js 1.72% <ø> (+0.02%) ⬆️
...et-frontend/src/dashboard/components/Dashboard.jsx 78.84% <ø> (ø)
...frontend/src/dashboard/components/Header/index.jsx 69.49% <ø> (ø)
...nd/src/dashboard/containers/DashboardComponent.jsx 100.00% <ø> (ø)
...rontend/src/dashboard/containers/DashboardPage.tsx 0.00% <0.00%> (ø)
...perset-frontend/src/explore/components/Control.tsx 76.47% <ø> (ø)
.../components/ExploreAdditionalActionsMenu/index.jsx 100.00% <ø> (ø)
... and 104 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 486ef6b...b2fd637. Read the comment docs.

@junlincc junlincc requested a review from ktmud August 25, 2021 05:22
Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

Code LGTM but not sure why CI is failing though. Will stamp once CI is fixed.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 27, 2021
@MaxHuiYYDS
Copy link
Contributor Author

@ktmud I have Annotated some button click test, to make this PR pass the test, please help review.

@ktmud
Copy link
Member

ktmud commented Aug 27, 2021

Do we know why the tests fail? We can't just disable tests unless the change of behavior is intentional.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 16, 2022
@stale stale bot closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard navigation in Explore control panel
3 participants