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

fix: Floating Menu in SQL Left Bar #13858

Merged
merged 22 commits into from Apr 5, 2021
Merged

Conversation

AAfghahi
Copy link
Member

@AAfghahi AAfghahi commented Mar 29, 2021

SUMMARY

The table and database selector were floating in SQL Left Bar. This solution fixes that by creating a new prop in TableSelector that determines whether to use a created ref or the document body.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

image
image

After:
Screen Shot 2021-03-29 at 5 27 25 PM
Screen Shot 2021-03-29 at 5 27 29 PM

TEST PLAN

This was visually tested.

ADDITIONAL INFORMATION

@AAfghahi
Copy link
Member Author

@junlincc and oh weird I can't tag yousoph. :/

@junlincc
Copy link
Member

/testenv up

@codecov
Copy link

codecov bot commented Mar 30, 2021

Codecov Report

Merging #13858 (91b12dd) into master (7621010) will increase coverage by 0.10%.
The diff coverage is 86.66%.

❗ Current head 91b12dd differs from pull request most recent head ee5ea3a. Consider uploading reports for the commit ee5ea3a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13858      +/-   ##
==========================================
+ Coverage   78.25%   78.35%   +0.10%     
==========================================
  Files         934      935       +1     
  Lines       47320    47405      +85     
  Branches     5937     5982      +45     
==========================================
+ Hits        37028    37146     +118     
+ Misses      10148    10116      -32     
+ Partials      144      143       -1     
Flag Coverage Δ
cypress 56.05% <74.15%> (+0.02%) ⬆️
hive ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...tend/src/SqlLab/components/ScheduleQueryButton.tsx 28.12% <ø> (ø)
...et-frontend/src/SqlLab/components/TableElement.jsx 88.60% <ø> (+0.23%) ⬆️
...end/src/common/components/DropdownButton/index.tsx 24.00% <0.00%> (ø)
...rset-frontend/src/components/ProgressBar/index.tsx 100.00% <ø> (ø)
.../components/CrossFilterScopingModal/utils/index.ts 100.00% <ø> (ø)
...src/dashboard/components/PublishedStatus/index.jsx 100.00% <ø> (ø)
...dashboard/components/SliceHeaderControls/index.jsx 77.89% <ø> (ø)
...perset-frontend/src/datasource/DatasourceModal.tsx 49.46% <ø> (-23.12%) ⬇️
...components/controls/FilterBoxItemControl/index.jsx 73.58% <ø> (ø)
...onents/controls/FilterControl/AdhocFilter/index.js 96.92% <ø> (ø)
... and 63 more

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 7621010...ee5ea3a. Read the comment docs.

Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can accomplish this with overflow: visible in a few places.

@ktmud
Copy link
Member

ktmud commented Mar 30, 2021

I also noticed that the menu overlaps with SQL Editor control buttons when table names are very long. Can you make sure this is also fixed?

messed-up

@github-actions
Copy link
Contributor

@junlincc Ephemeral environment spinning up at http://34.209.221.220:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@AAfghahi
Copy link
Member Author

@ktmud

Oh wow that looks weird. Thanks for bringing it up. I will need to find some databases that match that length and test.

@etr2460
Copy link
Member

etr2460 commented Apr 1, 2021

We ran into the bug this is fixing in our weekly release today. Since this PR is still in progress and the PR that introduced the bug reverts cleanly (and easily) how do we feel about reverting #13694?

cc @AAfghahi @yousoph @junlincc

@AAfghahi
Copy link
Member Author

AAfghahi commented Apr 1, 2021

@etr2460 that sounds good to me, this should be merged soon.

@AAfghahi
Copy link
Member Author

AAfghahi commented Apr 1, 2021

I also noticed that the menu overlaps with SQL Editor control buttons when table names are very long. Can you make sure this is also fixed?

messed-up

Hello @ktmud how does this look:
Screen Shot 2021-04-01 at 5 42 09 PM
You could still use the tooltip to see the full name.

@eschutho
Copy link
Member

eschutho commented Apr 2, 2021

This looks good @AAfghahi. Were you able to get the ellipses to show up? I didn't see them.

@AAfghahi
Copy link
Member Author

AAfghahi commented Apr 2, 2021

@eschutho no, I think I need to set a definable width.

@eschutho
Copy link
Member

eschutho commented Apr 2, 2021

I don't have a huge preference whether we have the ellipsis or not.. @Steejay wdyt?

@Steejay
Copy link

Steejay commented Apr 2, 2021

@AAfghahi @eschutho hello! Ideally we'd add an ellipsis so that the user knows that the copy is cut off. This will then help indicate that a tooltip would reveal the complete copy.

@AAfghahi
Copy link
Member Author

AAfghahi commented Apr 2, 2021

Screen.Recording.2021-04-02.at.2.07.07.PM.mov

I think this is everything that we wanted.

Copy link
Member

@eschutho eschutho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @AAfghahi!
Needs visual testing still.

@ktmud
Copy link
Member

ktmud commented Apr 5, 2021

@Steejay I think it's OK for the dropdown menu to be wider than the select box (it shouldn't be shorter than it though).

Even AntD has a dropdownMatchSelectWidth option which you can set to false to allow this auto-expand:
https://codesandbox.io/s/search-with-sort-antd4150-forked-nkude?file=/index.js

I just removed the fixed width of the menu container when there is no need to use virtual list.

@AAfghahi This looks good. Could you also add screenshots of how the select look like in Explore page chart controls and Dashboard FilterBox just for confirmation?

@betodealmeida
Copy link
Member

/testenv up

@AAfghahi
Copy link
Member Author

AAfghahi commented Apr 5, 2021

@ktmud I don't have any filter boxes with really long names.
Screen Shot 2021-04-05 at 1 02 46 PM

Screen Shot 2021-04-05 at 1 12 12 PM

Is there a public database that does so I can check it?

@AAfghahi
Copy link
Member Author

AAfghahi commented Apr 5, 2021

Screen Shot 2021-04-05 at 1 52 38 PM

ok found one

but also, the change to fix this was specific to SQL Editor. I think that the z-indexes of the other portions it is used are not so varied.

@betodealmeida
Copy link
Member

betodealmeida commented Apr 5, 2021

@ktmud I don't have any filter boxes with really long names.
Is there a public database that does so I can check it?

You can just add a row. Go to SQL Lab and run something like this:

UPDATE table SET country = 'XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX' WHERE country = 'Angola';

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2021

@betodealmeida Ephemeral environment creation is currently limited to committers.

@AAfghahi
Copy link
Member Author

AAfghahi commented Apr 5, 2021

@ktmud
Screen Shot 2021-04-05 at 3 36 35 PM
Screen Shot 2021-04-05 at 3 35 30 PM

@ktmud
Copy link
Member

ktmud commented Apr 5, 2021

@AAfghahi Thank you for the screenshots!

@hughhhh
Copy link
Member

hughhhh commented Apr 5, 2021

/testenv up

@eschutho
Copy link
Member

eschutho commented Apr 5, 2021

@yousoph did visual testing
🏷 ready-to-merge

@superset-github-bot superset-github-bot bot added the need:merge The PR is ready to be merged label Apr 5, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2021

@hughhhh Ephemeral environment spinning up at http://54.191.245.80:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@betodealmeida betodealmeida merged commit f19f2c3 into apache:master Apr 5, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2021

Ephemeral environment shutdown and build artifacts deleted.

@@ -300,6 +300,7 @@ div.Workspace {
.schemaPane-enter-done,
.schemaPane-exit {
transform: translateX(0);
z-index: 1020;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

henryyeh pushed a commit to preset-io/superset that referenced this pull request Apr 6, 2021
* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* table values

* aligned

* active key

* changed to customStyle

* update dropdown styles

Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
(cherry picked from commit f19f2c3)
lyndsiWilliams pushed a commit to preset-io/superset that referenced this pull request Apr 7, 2021
* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* table values

* aligned

* active key

* changed to customStyle

* update dropdown styles

Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* table values

* aligned

* active key

* changed to customStyle

* update dropdown styles

Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* floating table git issue

* made changes

* floating table git issue

* made changes

* long table names

* table values

* aligned

* active key

* changed to customStyle

* update dropdown styles

Co-authored-by: Elizabeth Thompson <eschutho@gmail.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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 need:merge The PR is ready to be merged size/S 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants