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

feat: minor reorder SQL Lab Tab controls #10257

Merged

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Jul 8, 2020

SUMMARY

A few airbnb users reported that they accidentally clicked status icon, 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:
Screen Shot 2020-07-08 at 10 56 39 AM

After:
Screen Shot 2020-07-08 at 9 36 37 AM

TEST PLAN

Integration test and manual test.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2020

Codecov Report

Merging #10257 into master will decrease coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            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%) ⬇️
#python 69.68% <ø> (ø)
Impacted Files Coverage Δ
...rontend/src/SqlLab/components/TabbedSqlEditors.jsx 74.02% <50.00%> (ø)
...t-frontend/src/SqlLab/components/TabStatusIcon.jsx 100.00% <100.00%> (+14.28%) ⬆️
...set-frontend/src/views/datasetList/DatasetList.tsx 68.25% <0.00%> (ø)

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 baeacc3...a1aa2c8. Read the comment docs.

@ktmud
Copy link
Member

ktmud commented Jul 8, 2020

image

Maybe separate to this, can we add a close icon for table preview tabs as well?

@graceguo-supercat
Copy link
Author

  • it's totally separated component and functions.
  • you can close preview from control panel:

L1rDIBpWyL

@rusackas
Copy link
Member

rusackas commented Jul 8, 2020

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?

@ktmud
Copy link
Member

ktmud commented Jul 8, 2020

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.

@mistercrunch
Copy link
Member

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.

@graceguo-supercat
Copy link
Author

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 :)

@graceguo-supercat graceguo-supercat merged commit 6690963 into apache:master Jul 8, 2020
@kenchendesign
Copy link

+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.

@graceguo-supercat
Copy link
Author

Sorry i clicked merge button accidentally. How can it get merged without code review approval?

@graceguo-supercat
Copy link
Author

So let's discuss how to improve it and I will make another PR for additional comments. Thanks!

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants