chore: Enhance Omnibar #16273
chore: Enhance Omnibar #16273
Conversation
…issue_15539_omnibar
…est.tsx Co-authored-by: Evan Rusackas <evan@preset.io>
…superset into chore/issue_16198_omnibar
…e/issue_16198_omnibar
…e/issue_16198_omnibar
…e/issue_16198_omnibar
Codecov Report
@@ Coverage Diff @@
## master #16273 +/- ##
==========================================
- Coverage 76.47% 76.47% -0.01%
==========================================
Files 997 997
Lines 53247 53260 +13
Branches 6777 6778 +1
==========================================
+ Hits 40722 40730 +8
- Misses 12295 12300 +5
Partials 230 230
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://54.201.229.97:8080. Credentials are |
|
The arrow navigation is conflicting with the mouse hover. In this example, I hovered an element with the mouse (dark gray) and then navigate to another option using the arrow keys (light gray). When I pressed Enter the mouse hovered option was selected. Screen.Recording.2021-08-16.at.3.56.09.PM.mov |
|
@michael-s-molina to be honest that looks like an edge case. I will check whether there is even a solution for that. However, this problem not related to my changes so I would get these changes merged in first if there isn't any problem related to the PR itself. |
@geido I think we can improve the criteria to split the PRs. I completely understand that sometimes we have reviews that suggest new features or completely different matters to the PR's intentions. On the other hand, we want to avoid a bureaucratic process where all that matters is the changed lines and the context is irrelevant. In my comment above, if you have caught that problem during your tests you probably would have added the fix in this PR, so to me, it belongs to the same context. We can do it in a separate PR but the fragmentation of the context is also something we need to consider when deciding if we should split the PRs. Getting the changes merged should be balanced with the overhead of filling new issues, collecting screenshots, and reloading the context in a follow-up review. |
|
@michael-s-molina I totally agree with your point. We shouldn't be bureaucratic and lose the context. I didn't want to convey that with my response. For this issue specifically, I was considering what you have reported as an edge case and I didn't it think would be a problem tackling it separately. I had a look anyway and found out that the Omnibar plugin that we are using comes with several limitations. I couldn't find a working solution so far, other than rendering the dropdown as a custom component. I have suggested before to go for a full refactor of this component and just build our own Omnibar. This would be much faster than tackling each case individually and would give us much more freedom. I advocate that we take that direction and I'll be happy to take that work on myself. |
@geido Agreed. Thanks for the explanation! |
|
Ephemeral environment shutdown and build artifacts deleted. |
* Fix style and implement ESC * Include ESC test case * Move pagination outside of table * Update superset-frontend/src/components/OmniContainer/OmniContainer.test.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * Enhance * Handle close * Localize placeholder * Fix tests * Clear input on close * Destroy modal on close * Clean up * Fix tests Co-authored-by: Evan Rusackas <evan@preset.io>
* Fix style and implement ESC * Include ESC test case * Move pagination outside of table * Update superset-frontend/src/components/OmniContainer/OmniContainer.test.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * Enhance * Handle close * Localize placeholder * Fix tests * Clear input on close * Destroy modal on close * Clean up * Fix tests Co-authored-by: Evan Rusackas <evan@preset.io>
mistercrunch
added
🏷️ bot
SUMMARY
This PR:
Video.Game.Sales.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION