You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/08/16 14:44:31 UTC
[GitHub] [superset] geido opened a new pull request #16273: chore: Enhance Omnibar
geido opened a new pull request #16273:
URL: https://github.com/apache/superset/pull/16273
### SUMMARY
This PR:
- Makes the Omnibar appear on the Dashboards' list with _cmd + k_ or _ctrl + k_
- Clears up the search when the Omnibar is closed
- Disables autocomplete
- Closes the Omnibar when clicking outside
https://user-images.githubusercontent.com/60598000/129581716-efc4100b-2156-4db6-bac6-1a9e68d25c2c.mp4
### TESTING INSTRUCTIONS
1. Open the dashboards' list
2. Press _cmd + k_ or _ctrl + k_ on your keyboard
3. Search for dashboards and move with the arrows through the results
4. Close the Omnibar by pressing ESC
5. Reopen the modal and observe that the latest search input and options are gone
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: #16198
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido edited a comment on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido edited a comment on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-900347034
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-900224189
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido edited a comment on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido edited a comment on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-900226196
Hello @michael-s-molina . I totally agree with your comment. However, this doesn't appear to be such a case. It is clearly an edge case, definitely unrelated to the intent of my PR and shouldn't be blocking the priority items that are instead intended in this PR. If you would like to work on that case immediately if you believe it is urgent, please feel free to do so. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido edited a comment on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido edited a comment on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-899886835
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido edited a comment on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido edited a comment on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-899886835
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-904453452
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-900347034
@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 happy to take that work on me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-899645094
/testenv up FEATURE_OMNIBAR=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-899646247
@geido Ephemeral environment spinning up at http://54.201.229.97:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-899886835
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-900351570
> 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-900226196
Hello @michael-s-molina . I totally agree with your comment. However, this doesn't appear to be such a case. It is clearly an edge case, definitely unrelated to the intent of my PR and shouldn't be blocking the priority items that are instead intended in this PR. If you would like to work on that case, please feel free to do so. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido removed a comment on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido removed a comment on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-900226196
Hello @michael-s-molina . I totally agree with your comment. However, this doesn't appear to be such a case. It is clearly an edge case, definitely unrelated to the intent of my PR and shouldn't be blocking the priority items that are instead intended in this PR. If you would like to work on that case immediately if you believe it is urgent, please feel free to do so. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-899642699
# [Codecov](https://codecov.io/gh/apache/superset/pull/16273?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#16273](https://codecov.io/gh/apache/superset/pull/16273?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b572957) into [master](https://codecov.io/gh/apache/superset/commit/ee9a384758790ffa7f216231a7751bc0c2970752?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ee9a384) will **decrease** coverage by `0.00%`.
> The diff coverage is `77.27%`.
> :exclamation: Current head b572957 differs from pull request most recent head acbe60f. Consider uploading reports for the commit acbe60f to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/16273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ 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
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `70.93% <77.27%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/16273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/Modal/Modal.tsx](https://codecov.io/gh/apache/superset/pull/16273/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTW9kYWwvTW9kYWwudHN4) | `100.00% <ø> (ø)` | |
| [...-frontend/src/components/OmniContainer/Omnibar.tsx](https://codecov.io/gh/apache/superset/pull/16273/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9PbW5pYmFyLnRzeA==) | `100.00% <ø> (ø)` | |
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/superset/pull/16273/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `78.84% <ø> (ø)` | |
| [...et-frontend/src/components/OmniContainer/index.tsx](https://codecov.io/gh/apache/superset/pull/16273/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvT21uaUNvbnRhaW5lci9pbmRleC50c3g=) | `86.95% <76.19%> (-10.11%)` | :arrow_down: |
| [...rontend/src/views/CRUD/dashboard/DashboardList.tsx](https://codecov.io/gh/apache/superset/pull/16273/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGFzaGJvYXJkL0Rhc2hib2FyZExpc3QudHN4) | `71.83% <100.00%> (+0.19%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16273?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16273?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ee9a384...acbe60f](https://codecov.io/gh/apache/superset/pull/16273?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #16273:
URL: https://github.com/apache/superset/pull/16273#issuecomment-899744483
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.
https://user-images.githubusercontent.com/70410625/129615418-15ff5dd0-0943-4c43-b9d0-3cd85e40bf5b.mov
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido merged pull request #16273: chore: Enhance Omnibar
Posted by GitBox <gi...@apache.org>.
geido merged pull request #16273:
URL: https://github.com/apache/superset/pull/16273
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org