-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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(Dashboard): Omnibar dropdown visibility and keyboard commands #16168
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16168 +/- ##
==========================================
- Coverage 76.83% 76.75% -0.08%
==========================================
Files 996 997 +1
Lines 53060 53201 +141
Branches 6766 6765 -1
==========================================
+ Hits 40766 40836 +70
- Misses 12066 12135 +69
- Partials 228 230 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up FEATURE_OMNIBAR=true |
@geido Ephemeral environment spinning up at http://34.209.163.80:8080. Credentials are |
1 - I wasn't able to open the Omnibar from the Dashboards page, only when entering a particular dashboard. 2 - When I open the bar again it's showing the search field with my previous search. Shouldn't be empty (initial state)? 3- I wasn't able to select one dashboard using the arrow keys because of the auto-complete. Can we disable it for this case? Screen.Recording.2021-08-10.at.4.09.34.PM.mov |
Thanks for your feedback @michael-s-molina I agree with all your points. However, those are all enhancements which aren't part of the original issue. I can open separate PRs for those depending on priority but let's get the original issue out of the way first. Thank you! |
…issue_15539_omnibar
Hey @michael-s-molina, I have captured those enhancements in this issue #16198 |
Im inclined to let this one in asap for the quick fix. then we can work on the enhancement Thank you so much for the improvement and suggestion! :) @geido @michael-s-molina |
@@ -84,40 +84,6 @@ test('Open Omnibar with ctrl + k with featureflag enabled', () => { | |||
).not.toBeVisible(); | |||
}); | |||
|
|||
test('Open Omnibar with ctrl + s with featureflag enabled', () => { |
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.
It seems like this test could be written with K
and esc
keystrokes, to keep it rather than delete it.
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.
Functional implementation seems fine, but can we keep the test with only slight modifications?
superset-frontend/src/components/OmniContainer/OmniContainer.test.tsx
Outdated
Show resolved
Hide resolved
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.
Sorry for the misconception about the test... this looks fine.
…est.tsx Co-authored-by: Evan Rusackas <[email protected]>
Ephemeral environment shutdown and build artifacts deleted. |
…gies * upstream/master: (64 commits) check roles before fetching reports (#16260) chore: upgrade mypy and add type guards (#16227) fix: pivot columns with ints for name (#16259) chore(pylint): Bump Pylint to 2.9.6 (#16146) fix examples tab for dashboard (#16253) chore: bump superset-ui packages to 0.17.84 (#16251) chore: Shows the dataset description in the gallery dropdown (#16200) fix(Dashboard): Omnibar dropdown visibility and keyboard commands (#16168) chore: bump py version for integration test (#16213) fix: skip perms on query context update (#16250) refactor: external metadata fetch API (#16193) feat(dao): admin can remove self from object owners (#15149) fix(dashboard): cross filter chart highlight when filters badge icon clicked (#16233) fix: validate_parameters and query (#16241) fix: Remove Advanced Analytics tag for 2 charts (#16240) Revert "feat: Changing Dataset names (#16199)" (#16235) feat: Allow users to connect via legacy SQLA form (#16201) fix: remove encryption from db params (#16214) fix(Explore): Show the tooltip only when label does not fit the container in METRICS/FILTERS/GROUP BY/SORT BY of the DATA panel (#16060) Show/hide tooltips (#16192) ... # Conflicts: # superset/tasks/caching/cache_strategy.py
…ache#16168) * Fix style and implement ESC * Include ESC test case * Update superset-frontend/src/components/OmniContainer/OmniContainer.test.tsx Co-authored-by: Evan Rusackas <[email protected]> Co-authored-by: Evan Rusackas <[email protected]>
…ache#16168) * Fix style and implement ESC * Include ESC test case * Update superset-frontend/src/components/OmniContainer/OmniContainer.test.tsx Co-authored-by: Evan Rusackas <[email protected]> Co-authored-by: Evan Rusackas <[email protected]>
SUMMARY
This PR fixes a problem with the visibility of the Omnibar dropdown. It also implements the ability to close the Omnibar by clicking ESC. Finally, it removes the ability to open the Omnibar with the ctrl/cmd + s in order to stop it from interfering with the default browser functionality. The Omnibar can now be opened with ctrl/cmd + k.
Fixes: #15539
BEFORE
FCC.New.Coder.Su.mp4
AFTER
FCC.New.Coder.Su.1.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION