fix(sql lab): table selector should display all the selected tables #19257
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19257 +/- ##
==========================================
- Coverage 66.67% 66.67% -0.01%
==========================================
Files 1676 1676
Lines 64715 64735 +20
Branches 6506 6511 +5
==========================================
+ Hits 43151 43164 +13
- Misses 19878 19885 +7
Partials 1686 1686
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
diegomedina248
force-pushed
the
fix/table-selector-should-clear-once-selected
branch
from
59c4054
to
d56076b
Compare
|
This works, but it feels a little unique/strange in how it works compared to other Select menus. Would it make sense to have it be a multiple select, so that ALL your choices persist? Then removing an item from the selection could remove the preview just like clicking the close button above the field list itself (which is also not a consistent pattern in Superset, really). cc @yousoph we can let this through, but a little piece of me wants to tap the brakes and have a brief design thinking moment here. |
Agreed, but I think it's worth a bigger discussion, cause the database & schema are required to perform the query, but the table is not, so I think it shouldn't belong in the same place. The table is there to get the table schema, which is nice, but feels disconnected. However, I feel like the solution in the PR is a bit better than the current one, so while we discuss a better approach, it's worth considering. |
|
One other change that might help is that the most recently selected table should appear at the top of the list in the left panel, so you have more feedback after you select your table. These 2 changes together were how the left panel used to work, I believe - they both broke some time ago |
That's fixed by this (merged) PR. 🎉 |
There was a problem hiding this comment.
LGTM, but pinging @michael-s-molina and/or @geido who know the Select component quite well, in case they see an alternative approach.
I totally agree with this. The ability to quickly remove one or all items without scrolling and keeping the behavior consistent with other Selects is essential. Right now, the behavior is really weird. I also agree with Diego's assessment that things look disconnected and need a design review (it's already happening). I think we should apply @rusackas suggestion before merging. Pinging @jess-dillard and @kasiazjc to consider this discussion in the new designs. |
I think the "after" is really confusing (and I would not recommend it) as there is no easy/immediate way to remove the selected schema table and you cannot quickly preview the selected schema tables. Is there a reason why this cannot be a multiple select and we added this flow instead? |
|
Agree with @kasiazjc that this field should be a multi-select and we should show all selections in the input box. Typically this is done as displaying the selections as tags so users can quickly remove them from the input box, but I can't think of anywhere else in the app we have this pattern. |
@jess-dillard you mean, where/if do we have the multiselect with tags in the app? We do, in filters, so we already have this pattern. |
|
@kasiazjc Ah yes, I couldn't remember if/where we used it. I'd recommend using this pattern here as well, allowing the user to populate tags in the table input field as they multi-select tables. Then they can remove the table from the field and sidebar by either removing the tag from the field (which would remove the columns from the sidebar) or the columns from the sidebar (which would remove the tag from the field). Also agree with @yousoph that table columns should be added to the sidebar in reverse order as the user adds them (most recently added table appears first in the sidebar). |
There was a problem hiding this comment.
The people have spoken :)
@diegomedina248 let's change to multi-select. Thanks!
diegomedina248
force-pushed
the
fix/table-selector-should-clear-once-selected
branch
from
d56076b
to
0fa6677
Compare
diegomedina248
changed the title
|
@rusackas @yousoph @michael-s-molina @kasiazjc @jess-dillard Interaction updated as discussed in the thread. Thanks! |
|
@diegomedina248 Thanks for addressing the suggestions. The default behavior of the multi-select component is to keep the popup open while the user is doing the selections. In this PR, if I'm trying to select multiple tables, it's really annoying because every selection closes the popup. Is it possible to keep it open? It would be nice to set The changes to the TableSelector affected other modules. The dataset editor is not showing the selected table anymore. Screen.Recording.2022-03-28.at.8.02.27.AM.mov |
@michael-s-molina Thanks for the comments, addressed your suggestions there. On the multi-select behavior, the reason I originally collapsed the select after each selection was done considering that, in most cases, you would want to select just one. |
|
/testenv up |
|
@yousoph Ephemeral environment spinning up at http://34.219.242.154:8080. Credentials are |
|
I agree with @diegomedina248 here that in most cases you'll want to select just one table at a time. @jess-dillard what are your thoughts on the multi select component behaving differently here than in other places where it's used and closing once one item it selected? |
|
@yousoph @diegomedina248 I think it should behave consistently with other multi-selects – if there's not a need to view more than one table at once, then we should use a single select and change the sidebar information every time a new table is selected. |
|
Ok! I think multi-select makes more sense than single select here so users can see multiple tables / previews, so I think the behavior as you have it works for the selector. |
|
Ephemeral environment shutdown and build artifacts deleted. |
…pache#19257) * fix: table Selector should clear once selected * Multi select * Add tests * refactor * PR comments
mistercrunch
added
🏷️ bot
SUMMARY
On the left panel in the SQL Lab editor, we can select a db, schema & table combination and it will query the schema for us.
Initial change
This PR updates the UI interaction so that, once a table is selected, is not persisted in that select. The reason being a user can select multiple tables and the schemas are all shown below.
After discussions
This PR updates the UI interaction so that the table select display all the selected tables, as opposed to clearing it.
Now, both the table select & the table list below are linked together bidirectionally and in sync.
The select is closed after each selection, considering that most of the time the user will want to pick a single table to work on (interaction up for debate).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Screen.Recording.2022-03-18.at.12.11.03.mov
After:
Screen.Recording.2022-03-25.at.19.06.23.mov
TESTING INSTRUCTIONS
Ensure after each table selection, the schema appears below (it appears at the end, so you might need to scroll)
Ensure the select contains all the selected tables, and that both the select. & list have the same tables.
ADDITIONAL INFORMATION