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 2020/07/08 17:30:45 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #10257: feat: minor reorder SQL Lab Tab controls
graceguo-supercat opened a new pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257
### SUMMARY
A few airbnb users reported that they accidentally clicked status icon in SQL Lab's tab, and they lost the active query.
This PR is to re-order the controls in each SQL Lab tab to prevent such accidents.
- re-order dropdown menu and tab title
- separate the status icon and close button
- always show close button at the right-side
Thanks for @kenchendesign brought up design!
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
**Before:**
<img width="429" alt="Screen Shot 2020-07-07 at 4 11 32 PM" src="https://user-images.githubusercontent.com/27990562/86950183-b0e44000-c104-11ea-8118-c0493a57c8be.png">
**After:**
<img width="825" alt="Screen Shot 2020-07-08 at 9 36 37 AM" src="https://user-images.githubusercontent.com/27990562/86950166-a75ad800-c104-11ea-8f6a-0ef9dd983409.png">
### TEST PLAN
Manual test.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [X] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] 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.
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] [incubator-superset] kenchendesign commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
kenchendesign commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655807396
+1 on making the `x` slightly thinner, I don't think they should be lighter-colored though.
+1 on making all tabs visually consistent with an `x` on the right end.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat merged pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257
----------------------------------------------------------------
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.
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] [incubator-superset] rusackas commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655774710
I'm in total agreement that the status dot acting as a close button is a dangerous pattern.
If closing the tab is a risk, would it be reasonable to remove the X button from the new design as well, and require that users click the ⠇menu and select "close tab" to close the tab?
If that's reasonable, I think the menu (whether a ⠇or a ˅ ) would be better back on the right side of the tab. I'm a bit ambivalent about whether the status dot should be to the left or right of the title.
**In other words** would it make sense to just remove the "X" close button from the status dot and call it a day?
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655750287
- it's totally separated component and functions.
- you can close preview from control panel:
----------------------------------------------------------------
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.
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] [incubator-superset] ktmud commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655775134
Yeah, I know that close button exists. Just a suggestion. Haven't used SQL Lab for a while and was surprised yesterday that it's not obvious how to close the preview tab.
> **In other words** would it make sense to just remove the "X" close button from the status dot and call it a day?
+1 to this, was just typing the same thing.
----------------------------------------------------------------
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.
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] [incubator-superset] ktmud commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655701620
Maybe separate to this, can we add a close icon for table preview tabs as well?
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-commenter commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655665743
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=h1) Report
> Merging [#10257](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/baeacc3c560dbd2ac9543912ca5559e112118d68&el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `80.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10257 +/- ##
==========================================
- Coverage 65.41% 65.41% -0.01%
==========================================
Files 598 598
Lines 32014 32003 -11
Branches 3237 3236 -1
==========================================
- Hits 20943 20935 -8
+ Misses 10890 10887 -3
Partials 181 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `59.28% <80.00%> (-0.02%)` | :arrow_down: |
| #python | `69.68% <ø> (ø)` | |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `74.02% <50.00%> (ø)` | |
| [...t-frontend/src/SqlLab/components/TabStatusIcon.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYlN0YXR1c0ljb24uanN4) | `100.00% <100.00%> (+14.28%)` | :arrow_up: |
| [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `69.04% <0.00%> (+0.79%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=footer). Last update [baeacc3...f1eb7b8](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-commenter edited a comment on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655665743
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=h1) Report
> Merging [#10257](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/baeacc3c560dbd2ac9543912ca5559e112118d68&el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `80.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10257 +/- ##
==========================================
- Coverage 65.41% 65.41% -0.01%
==========================================
Files 598 598
Lines 32014 32003 -11
Branches 3237 3236 -1
==========================================
- Hits 20943 20935 -8
+ Misses 10890 10887 -3
Partials 181 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `59.28% <80.00%> (-0.02%)` | :arrow_down: |
| #python | `69.68% <ø> (ø)` | |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `74.02% <50.00%> (ø)` | |
| [...t-frontend/src/SqlLab/components/TabStatusIcon.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYlN0YXR1c0ljb24uanN4) | `100.00% <100.00%> (+14.28%)` | :arrow_up: |
| [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `69.04% <0.00%> (+0.79%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=footer). Last update [baeacc3...f1eb7b8](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-commenter edited a comment on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655665743
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=h1) Report
> Merging [#10257](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/baeacc3c560dbd2ac9543912ca5559e112118d68&el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `80.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10257 +/- ##
==========================================
- Coverage 65.41% 65.41% -0.01%
==========================================
Files 598 598
Lines 32014 32003 -11
Branches 3237 3236 -1
==========================================
- Hits 20943 20934 -9
+ Misses 10890 10888 -2
Partials 181 181
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `59.28% <80.00%> (-0.02%)` | :arrow_down: |
| #python | `69.68% <ø> (ø)` | |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `74.02% <50.00%> (ø)` | |
| [...t-frontend/src/SqlLab/components/TabStatusIcon.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYlN0YXR1c0ljb24uanN4) | `100.00% <100.00%> (+14.28%)` | :arrow_up: |
| [...set-frontend/src/views/datasetList/DatasetList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10257/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2RhdGFzZXRMaXN0L0RhdGFzZXRMaXN0LnRzeA==) | `68.25% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=footer). Last update [baeacc3...a1aa2c8](https://codecov.io/gh/apache/incubator-superset/pull/10257?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655806222
Generally it's probably best to align with the way browser tabs behave as the most commonly used tabs out there.
Visually the `x` is a bit bold and large. Maybe we can mute it a little (?) - sorry to ask for CSS adjustments on a PR... Could also only show the `x` for the active tab to make things more busy (?)
Stepping back, I think the goal longer term is still to get rid of in-app tabs and push users to use browser tabs instead.
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655809354
Sorry i clicked merge button accidentally. How can it get merged without code review approval?
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655807234
I like to keep `x` as short-cut in the sql lab tab...especially when you want to close many tabs. click drop-down then click close need a lot more...work :)
----------------------------------------------------------------
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.
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] [incubator-superset] graceguo-supercat commented on pull request #10257: feat: minor reorder SQL Lab Tab controls
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10257:
URL: https://github.com/apache/incubator-superset/pull/10257#issuecomment-655810077
So let's discuss how to improve it and I will make another PR for additional comments. Thanks!
----------------------------------------------------------------
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.
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