fix(sqllab): Handle long table names in SQL Lab #10518

suddjian · 2020-08-04T21:11:38Z

SUMMARY

In cases where there are lots of tables and/or tables with long names, SQL Lab can become difficult to use. This adds a couple of affordances for that use case:

Adds tooltips to items in the table select menu, displaying the full table name
Widen the autocomplete window. Unfortunately I couldn't find a way to make the width dynamic or add a tooltip. But it's now twice as wide as before which should be pretty accommodating.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

etr2460 · 2020-08-04T23:10:09Z

How does the width look when the tables don't have long names? is it too ridiculous?

mistercrunch · 2020-08-05T04:59:40Z

Mmmmh, it feels like the tooltip should only be where needed. Really unusual to see a tooltip in a dropdown list... I searched a bit to see if react-select could expand the width of the dropdown as needed but didn't find anything obvious. Seems like the dropdown box could be wider than the non-expanded control.

rusackas · 2020-08-05T05:27:59Z

🏷 sqllab

rusackas · 2020-08-05T05:28:30Z

🗑🏷 sqllab

mistercrunch · 2020-08-05T05:44:50Z

🏷️ .ui

rusackas · 2020-08-05T05:55:36Z

superset-frontend/src/SqlLab/components/AceEditorWrapper/AceStyles.less

+ */
+
+.ace_editor.ace_autocomplete {
+  width: 520px;


Not sure we need a new file here... this could be shoehorned under the .ace_editor selector in main.less

Optionally, all o' that could be moved into Emotion (though that comes with the side mission of migrating the ligature settings into the SupersetTheme.

Also, is 520px based on... something? Maybe it could be a percentage?

Adding it to main.less makes sense to me. That would change autocomplete widths for every instance of ace editor that we have.

I chose 520 based on what looked good to me on-screen. Can't be a percentage because the element is absolute positioned. We could use vw, but I don't really like how that can't be bounded.

superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx

suddjian · 2020-08-05T16:47:28Z

I don't think it looks too ridiculous

….tsx Co-authored-by: Evan Rusackas <evan@preset.io>

rusackas · 2020-08-05T17:00:04Z

In your second screenshot it looks like you might be able to use some CSS to truncate the list item with an ellipsis, e.g.

white-space: nowrap; // probably already happening
overflow: hidden; // _maybe_ already happening
text-overflow: ellipsis; // …

suddjian · 2020-08-05T17:08:50Z

Mmmmh, it feels like the tooltip should only be where needed. Really unusual to see a tooltip in a dropdown list... I searched a bit to see if react-select could expand the width of the dropdown as needed but didn't find anything obvious. Seems like the dropdown box could be wider than the non-expanded control.

I agree that having a dynamic dropdown width would be best. I couldn't find a way to make that work either, at least not without going and changing the react-select library. There are actually cases where the dropdown is rendered wide enough to accommodate the longer table names. It only happens when the long name appears in the first few results, because the options are virtualized. It might be possible to figure out up front what the widest option will be and set that as the width, but you'd have to do some pretty weird stuff to get that to work.

I think the best way to improve this would be to give SQL Lab a thorough design pass. A dropdown might not actually be the best way to solve this user need.

suddjian · 2020-08-05T17:11:10Z

In your second screenshot it looks like you might be able to use some CSS to truncate the list item with an ellipsis, e.g.
white-space: nowrap; // probably already happening
overflow: hidden; // _maybe_ already happening
text-overflow: ellipsis; // …

Interestingly, the dropdown is actually horizontally scrollable so this wouldn't work. In theory, the horizontal scrolling should solve the issue of long table names, but it's not a very discoverable pattern. Maybe a better solution would be to make the scrollbar clearly visible and get rid of the tooltips.

ktmud · 2020-08-05T19:10:22Z

I implemented the feature where long table names auto-expand the dropdown select. This is done only for non-virtualized list because it was easier. Didn't follow up to fix it for the virtualized list because once users start typing and narrow the list down to < 100 items, it becomes a non-virtualized list and will auto-expand anyway.

We can potentially slightly increase the windowThreshold (currently defaults to 100) on Select to make table name truncating less frequent for users.

etr2460 · 2020-08-05T21:14:04Z

increasing the windowThreshold seems reasonable, since each dom element within the select isn't too complex. Maybe with some performance testing we can find an optimal value here?

ktmud · 2020-08-05T21:49:15Z

Another easy "fix" is instead of tooltips, we can add the less aggressive html title attribute to the table items.

suddjian · 2020-08-05T22:45:16Z

Oh it hadn't even occurred to me to use title, that's a good idea. I'll do a bit of testing with windowThreshold. Out of curiosity, how many tables do you guys have at Airbnb?

ktmud · 2020-08-05T23:04:45Z

It depends on the schema. Normally it'd be less than 100, but we also have schemas with 1000+ tables.

etr2460 · 2020-08-05T23:06:27Z

We've got a >10k table schema, but that's the largest we have so far i think

suddjian · 2020-08-08T01:15:00Z

I tried passing a custom windowThreshold to the <Select> but it didn't seem to have any effect. I profiled scrolling around the select menu with and without windowThreshold set to 10000 and with 10000+ tables in the schema. I don't think setting the threshold worked correctly. It was around 200ms to render the menu each time regardless, and it didn't seem like there was actually a larger window of menu items.

ktmud · 2020-08-08T01:50:36Z

I tried passing a custom windowThreshold to the <Select> but it didn't seem to have any effect. I profiled scrolling around the select menu with and without windowThreshold set to 10000 and with 10000+ tables in the schema. I don't think setting the threshold worked correctly. It was around 200ms to render the menu each time regardless, and it didn't seem like there was actually a larger window of menu items.

Did your IDE give you type hint?

because it worked for me. There is also a menuIsOpen option you can set to always open the menu on load, so you can more easily see what is rendered by inspecting elements.

Base on my experience, the scrolling starts to become janky even with just 200+ items, which is why I picked 100 as default in the first place.

codecov-commenter · 2020-08-10T05:02:52Z

Codecov Report

Merging #10518 into master will decrease coverage by 4.64%.
The diff coverage is 70.16%.

@@            Coverage Diff             @@
##           master   #10518      +/-   ##
==========================================
- Coverage   70.74%   66.10%   -4.65%     
==========================================
  Files         604      605       +1     
  Lines       32391    32430      +39     
  Branches     3282     3426     +144     
==========================================
- Hits        22916    21438    -1478     
- Misses       9363    10802    +1439     
- Partials      112      190      +78

Flag	Coverage Δ
#cypress	`?`
#javascript	`59.94% <93.33%> (+0.22%)`	⬆️
#python	`70.41% <66.97%> (+0.11%)`	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/components/TableSelector.jsx	`79.08% <ø> (-5.89%)`	⬇️
...rontend/src/explore/components/PropertiesModal.tsx	`14.92% <0.00%> (-2.99%)`	⬇️
...et-frontend/src/views/CRUD/dataset/DatasetList.tsx	`73.18% <ø> (ø)`
superset/cli.py	`39.73% <0.00%> (+0.13%)`	⬆️
superset/connectors/druid/views.py	`69.86% <0.00%> (+0.47%)`	⬆️
superset/dashboards/schemas.py	`98.50% <ø> (ø)`
superset/queries/schemas.py	`100.00% <ø> (ø)`
superset/sql_lab.py	`78.69% <0.00%> (+0.77%)`	⬆️
superset/views/datasource.py	`93.44% <ø> (ø)`
superset/utils/core.py	`88.99% <25.00%> (-0.40%)`	⬇️
... and 193 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 5bb8b97...29f1a2d. Read the comment docs.

suddjian · 2020-08-10T16:24:13Z

@ktmud I believe that's exactly what I tried. Got the intellisense completion and everything. Maybe I did something subtly wrong somewhere.

I changed to title attribute as suggested. I think optimization around the select component could be implemented in another PR.

etr2460

lgtm, one comment to address in the future

etr2460 · 2020-08-11T00:34:08Z

superset-frontend/src/SqlLab/components/AceEditorWrapper.tsx

@@ -22,14 +22,14 @@ import 'brace/mode/sql';
 import 'brace/theme/github';
 import 'brace/ext/language_tools';
 import ace from 'brace';
-import { areArraysShallowEqual } from '../../reduxUtils';
-import sqlKeywords from '../utils/sqlKeywords';
+import { areArraysShallowEqual } from 'src/reduxUtils';


is this from a lint rule? auto modifications from your IDE? Could we do a programmatic migration here to absolute paths everywhere?

We probably could do that, but I'm not sure it's worth it. I think I made a decision to change it here because it made more sense than a local path. But sometimes local paths are nice.

* widen the autocomplete menu for table names * display the full table name in a tooltip * license * Update superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * src importing * move autocomplete width css to main.less * use html title attribute instead of tooltip Co-authored-by: Evan Rusackas <evan@preset.io>

suddjian added 2 commits August 3, 2020 23:36

widen the autocomplete menu for table names

19cb561

display the full table name in a tooltip

a9e91ef

suddjian requested review from rusackas and etr2460 August 4, 2020 21:11

superset-github-bot bot added the preset-io label Aug 4, 2020

pull-request-size bot added the size/M label Aug 4, 2020

suddjian changed the title ~~Handle long table names in SQL Lab~~ fix(sqllab): Handle long table names in SQL Lab Aug 4, 2020

license

dbbd54f

mistercrunch added the design-review label Aug 5, 2020

superset-github-bot bot added the sqllab label Aug 5, 2020

superset-github-bot bot removed the sqllab label Aug 5, 2020

rusackas reviewed Aug 5, 2020

View reviewed changes

superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx Outdated Show resolved Hide resolved

rusackas reviewed Aug 5, 2020

View reviewed changes

superset-frontend/src/SqlLab/components/AceEditorWrapper/index.tsx Outdated Show resolved Hide resolved

Update superset-frontend/src/SqlLab/components/AceEditorWrapper/index…

b15b8bb

….tsx Co-authored-by: Evan Rusackas <evan@preset.io>

suddjian added 2 commits August 5, 2020 10:14

src importing

8c1c08a

move autocomplete width css to main.less

a8db6ca

pull-request-size bot added size/S and removed size/M labels Aug 10, 2020

use html title attribute instead of tooltip

29f1a2d

suddjian force-pushed the fix/long-table-names branch from 6b698a0 to 29f1a2d Compare August 10, 2020 05:42

etr2460 approved these changes Aug 11, 2020

View reviewed changes

suddjian merged commit 83af9d1 into master Aug 11, 2020

suddjian deleted the fix/long-table-names branch September 1, 2020 22:53

villebro added the v0.38 label Sep 9, 2020

dpgaspar removed the v0.38 label Sep 10, 2020

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sqllab): Handle long table names in SQL Lab #10518

fix(sqllab): Handle long table names in SQL Lab #10518

suddjian commented Aug 4, 2020

etr2460 commented Aug 4, 2020

mistercrunch commented Aug 5, 2020

rusackas commented Aug 5, 2020

rusackas commented Aug 5, 2020

mistercrunch commented Aug 5, 2020

rusackas Aug 5, 2020

suddjian Aug 5, 2020 •

edited

Loading

suddjian commented Aug 5, 2020

rusackas commented Aug 5, 2020

suddjian commented Aug 5, 2020 •

edited

Loading

suddjian commented Aug 5, 2020

ktmud commented Aug 5, 2020

etr2460 commented Aug 5, 2020

ktmud commented Aug 5, 2020

suddjian commented Aug 5, 2020

ktmud commented Aug 5, 2020

etr2460 commented Aug 5, 2020

suddjian commented Aug 8, 2020

ktmud commented Aug 8, 2020 •

edited

Loading

codecov-commenter commented Aug 10, 2020 •

edited

Loading

suddjian commented Aug 10, 2020 •

edited

Loading

etr2460 left a comment

etr2460 Aug 11, 2020

suddjian Aug 11, 2020

fix(sqllab): Handle long table names in SQL Lab #10518

fix(sqllab): Handle long table names in SQL Lab #10518

Conversation

suddjian commented Aug 4, 2020

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

etr2460 commented Aug 4, 2020

mistercrunch commented Aug 5, 2020

rusackas commented Aug 5, 2020

rusackas commented Aug 5, 2020

mistercrunch commented Aug 5, 2020

rusackas Aug 5, 2020

Choose a reason for hiding this comment

suddjian Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

suddjian commented Aug 5, 2020

rusackas commented Aug 5, 2020

suddjian commented Aug 5, 2020 • edited Loading

suddjian commented Aug 5, 2020

ktmud commented Aug 5, 2020

etr2460 commented Aug 5, 2020

ktmud commented Aug 5, 2020

suddjian commented Aug 5, 2020

ktmud commented Aug 5, 2020

etr2460 commented Aug 5, 2020

suddjian commented Aug 8, 2020

ktmud commented Aug 8, 2020 • edited Loading

codecov-commenter commented Aug 10, 2020 • edited Loading

Codecov Report

suddjian commented Aug 10, 2020 • edited Loading

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Aug 11, 2020

Choose a reason for hiding this comment

suddjian Aug 11, 2020

Choose a reason for hiding this comment

suddjian Aug 5, 2020 •

edited

Loading

suddjian commented Aug 5, 2020 •

edited

Loading

ktmud commented Aug 8, 2020 •

edited

Loading

codecov-commenter commented Aug 10, 2020 •

edited

Loading

suddjian commented Aug 10, 2020 •

edited

Loading