-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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(explore): Upgraded viz select gallery #15303
Conversation
* feat(viz): add categories to the viz picker * fix test types * add a catch-all category * tweak layout * upgrade superset-ui to get new metadata * do i look like i know what a jpeg is * fix tests * lint * remove script count test requirement * fix e2e test
Codecov Report
@@ Coverage Diff @@
## master #15303 +/- ##
==========================================
- Coverage 76.95% 76.94% -0.01%
==========================================
Files 976 977 +1
Lines 51326 51852 +526
Branches 6912 7132 +220
==========================================
+ Hits 39498 39899 +401
- Misses 11607 11726 +119
- Partials 221 227 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* upgrade superset-ui, install fuse.js * add metadata to plugin context * get search working * layout improvements * fix tests * Update superset-frontend/src/explore/components/controls/VizTypeControl/index.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * use typography size instead of grid unit * comments Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
* feat(explore): add section for example images in viz type control * fix jpg webpack config * formatting
* separate viz gallery from the modal * use gallery directly in add slice view * more formatting on the add slice container * restyle the thumbnail list * explicit thumbnail width and height * remove crappy hack * remove useless line * comment * sort categories * comments * tweak search behavior * fix tests * open gallery to the currently selected viz type * null safety * show all plugins when searching empty string * get the new metadatas * adjust categories scrolling behavior * add time series table metadata * upgrade superset-ui * attempt fixing tests * upgrade descriptions * fix unit test * attempt fixing e2e again * max width for viz gallery
8cd4fe8
to
7ce2d7a
Compare
ce9784c
to
f2a0a1f
Compare
/testenv up |
@suddjian Ephemeral environment spinning up at http://34.220.80.11:8080. Credentials are |
First of all, this is looking great! Congrats I wanted to do some manual testing out of curiosity and came up with the following feedback. Selected viz typeWhen changing a viz type, the modal shows the correct title and description at the bottom. However, the selection of the category is incorrect and it is not showing the currently selected type. Inconsistent search behaviourWhen clicking on the search input, the category is automatically reset to the default. However, when clearing up the search the previously selected category gets selected back again. I think the behaviour should be consistent and clearing up should bring you to the default, not the previously selected category. Default orderDo we have a default order when searching? Should it be alphabetical? Unnecessary scroll barJust a minor thing. The scroll bar for the categories is showing up even though there are no other categories. |
/testenv up |
@rusackas Ephemeral environment spinning up at http://54.186.68.80:8080. Credentials are |
Thank you for your brilliant work, it looks amazing! 🚀 Here are a some very minor details I've picked along the way: Empty selection by default When creating a new graph, the first chart type (here, Table) is selected by default (cf. lower half of the dialog). It would feel more natural (to me) that no chart is selected by default. Selection not cleared when changing category In the same spirit, I'd like to be able to “clear” the selected visualization when changing category. Currently, the last selection “sticks” in the lower half of the dialog, even if that chart type isn't part of the currently selected Category (upper half). This can cause some confusion as I can have both “BigNumber” (selected chart type) and “Correlation” (selected category) at the same time… Charts categorization I fully understand this is not a trivial task and that no solution might satisfy everyone. Here are just some of the chart categorisation that appeared unnatural to me:
I appreciate the ranking is coherent with D3's one (and I even voted in favour of that ranking in SIP-67)… but the categorization still feels a bit unnatural to me (i.e. if I were to look for a given chart, I'm not sure I would find it first time/I wouldn't struggle finding it — btw, the search bar is a must have, thanks!). Browse through all charts at a glance I am aware this defeats a large part of your work with the categorization, but something I like with D3's gallery is that I can have all chart types at a glance by scrolling through one single page. Here, when one clicks on a category, it filters out all the other charts. Add “deprecated” label Some charts are due to be deprecated. This is detailed in their description, yet I feel this doesn't stand out enough. A grey “deprecated” tag (similar to tags in GitHub issues/PR) could help notice it more. Chart examples thumbnails scrollbar On Firefox DevEd 90.0b12 (64-bit), with ×150% zoom, some “non-scrollable scrollbars” appear around the chart example thumbnails area. In general, I would expect the thumbnails to be more flexible in their sizing (as it's not such an issue if they are downscaled, unlike the description text), so that they adapt to their container instead of forcing it to display scrollbars. Related to this, the chart description and thumbnail area are 2 separate components, with independent scrollbars. It would feel more natural to me to consider both elements as one unique “scrollable” entity. (I.e. have only one scrollbar on the far right, even if it's the text that overflows.) This would mean that “Example” become a sub-heading of the chart type (here “Heatmap”) — one could either introduce the “Description” subheading as well for the left column, or get rid of the “Example” one? All this being said, it's already such a huge step forward I'm very much looking forward to using in prod; thanks a lot! |
Thank you both very much for the well-written feedback, paging @junlincc to get product eyes on this. Some notes:
|
* start viz gallery with null selection, clear when switching categories * fix AddSliceContainer tests * show a message when there is no viz type selected * composition > inheritance * clarify searching code
392e857
to
e8beb43
Compare
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.208.15.163:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of good ideas for iteration/improvement on the thread, but to my eyes, this thing looks like a huge improvement, and looks ready to merge!
Not for interested parties, that additional PRs in superset-ui to add metadata to the rest of the plugins, are underway right now
@@ -113,6 +113,7 @@ | |||
"emotion-rgba": "0.0.9", | |||
"fontsource-fira-code": "^3.0.5", | |||
"fontsource-inter": "^3.0.5", | |||
"fuse.js": "^6.4.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but I'll bet there are at least a few other places in Superset it would be awesome to apply this.
value: ChartMetadata; | ||
}; | ||
|
||
const DEFAULT_ORDER = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how much value this DEFAULT_ORDER has anymore, but that discussion shouldn't block this PR.
() => selectedVizMetadata?.category || categories[0], | ||
); | ||
|
||
const fuse = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but this might warrant a comment to explain it's using this Fuse component for fuzzy-search here, to save folks a googling.
Ephemeral environment shutdown and build artifacts deleted. |
* add modal layout with description, rework styles * thirty percent * test correctly * avoid any changes in modal height * typescriptify * feat(viz): add categories to the viz picker (apache#15304) * feat(viz): add categories to the viz picker * fix test types * add a catch-all category * tweak layout * upgrade superset-ui to get new metadata * do i look like i know what a jpeg is * fix tests * lint * remove script count test requirement * fix e2e test * feat(explore): Viz picker search improvements (apache#15399) * upgrade superset-ui, install fuse.js * add metadata to plugin context * get search working * layout improvements * fix tests * Update superset-frontend/src/explore/components/controls/VizTypeControl/index.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * use typography size instead of grid unit * comments Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * feat(explore): Examples image gallery in the viz type control (apache#15489) * feat(explore): add section for example images in viz type control * fix jpg webpack config * formatting * feat(Explore): Viz gallery component tweaks (apache#15520) * separate viz gallery from the modal * use gallery directly in add slice view * more formatting on the add slice container * restyle the thumbnail list * explicit thumbnail width and height * remove crappy hack * remove useless line * comment * sort categories * comments * tweak search behavior * fix tests * open gallery to the currently selected viz type * null safety * show all plugins when searching empty string * get the new metadatas * adjust categories scrolling behavior * add time series table metadata * upgrade superset-ui * attempt fixing tests * upgrade descriptions * fix unit test * attempt fixing e2e again * max width for viz gallery * update package lock * undo unnecessary webpack changes * don't show search results until something is entered * force modal to open to selected viz type * tweaks to search behavior * gallery layout tweaks * enshrine pivot table v2 in a place of honor * feat(viz): Clear viz gallery when navigating between categories (apache#15577) * start viz gallery with null selection, clear when switching categories * fix AddSliceContainer tests * show a message when there is no viz type selected * composition > inheritance * clarify searching code * comment Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
* add modal layout with description, rework styles * thirty percent * test correctly * avoid any changes in modal height * typescriptify * feat(viz): add categories to the viz picker (apache#15304) * feat(viz): add categories to the viz picker * fix test types * add a catch-all category * tweak layout * upgrade superset-ui to get new metadata * do i look like i know what a jpeg is * fix tests * lint * remove script count test requirement * fix e2e test * feat(explore): Viz picker search improvements (apache#15399) * upgrade superset-ui, install fuse.js * add metadata to plugin context * get search working * layout improvements * fix tests * Update superset-frontend/src/explore/components/controls/VizTypeControl/index.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * use typography size instead of grid unit * comments Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * feat(explore): Examples image gallery in the viz type control (apache#15489) * feat(explore): add section for example images in viz type control * fix jpg webpack config * formatting * feat(Explore): Viz gallery component tweaks (apache#15520) * separate viz gallery from the modal * use gallery directly in add slice view * more formatting on the add slice container * restyle the thumbnail list * explicit thumbnail width and height * remove crappy hack * remove useless line * comment * sort categories * comments * tweak search behavior * fix tests * open gallery to the currently selected viz type * null safety * show all plugins when searching empty string * get the new metadatas * adjust categories scrolling behavior * add time series table metadata * upgrade superset-ui * attempt fixing tests * upgrade descriptions * fix unit test * attempt fixing e2e again * max width for viz gallery * update package lock * undo unnecessary webpack changes * don't show search results until something is entered * force modal to open to selected viz type * tweaks to search behavior * gallery layout tweaks * enshrine pivot table v2 in a place of honor * feat(viz): Clear viz gallery when navigating between categories (apache#15577) * start viz gallery with null selection, clear when switching categories * fix AddSliceContainer tests * show a message when there is no viz type selected * composition > inheritance * clarify searching code * comment Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
* add modal layout with description, rework styles * thirty percent * test correctly * avoid any changes in modal height * typescriptify * feat(viz): add categories to the viz picker (apache#15304) * feat(viz): add categories to the viz picker * fix test types * add a catch-all category * tweak layout * upgrade superset-ui to get new metadata * do i look like i know what a jpeg is * fix tests * lint * remove script count test requirement * fix e2e test * feat(explore): Viz picker search improvements (apache#15399) * upgrade superset-ui, install fuse.js * add metadata to plugin context * get search working * layout improvements * fix tests * Update superset-frontend/src/explore/components/controls/VizTypeControl/index.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * use typography size instead of grid unit * comments Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * feat(explore): Examples image gallery in the viz type control (apache#15489) * feat(explore): add section for example images in viz type control * fix jpg webpack config * formatting * feat(Explore): Viz gallery component tweaks (apache#15520) * separate viz gallery from the modal * use gallery directly in add slice view * more formatting on the add slice container * restyle the thumbnail list * explicit thumbnail width and height * remove crappy hack * remove useless line * comment * sort categories * comments * tweak search behavior * fix tests * open gallery to the currently selected viz type * null safety * show all plugins when searching empty string * get the new metadatas * adjust categories scrolling behavior * add time series table metadata * upgrade superset-ui * attempt fixing tests * upgrade descriptions * fix unit test * attempt fixing e2e again * max width for viz gallery * update package lock * undo unnecessary webpack changes * don't show search results until something is entered * force modal to open to selected viz type * tweaks to search behavior * gallery layout tweaks * enshrine pivot table v2 in a place of honor * feat(viz): Clear viz gallery when navigating between categories (apache#15577) * start viz gallery with null selection, clear when switching categories * fix AddSliceContainer tests * show a message when there is no viz type selected * composition > inheritance * clarify searching code * comment Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
SUMMARY
This feature branch had several PRs merged into it that were independently reviewed. Approving this should be pretty much a rubber stamp.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
![Screen Shot 2021-07-02 at 5 32 43 PM](https://user-images.githubusercontent.com/1858430/124338236-8aac8100-db5b-11eb-89bd-e1ab0e26f55b.png)
Initial Design:
![Screen Shot 2021-06-11 at 3 38 58 PM](https://user-images.githubusercontent.com/1858430/121755092-269e1c00-cacb-11eb-9635-469179760356.png)
After:
![Screen Shot 2021-07-02 at 5 31 42 PM](https://user-images.githubusercontent.com/1858430/124338208-694b9500-db5b-11eb-8029-ab0a7859b811.png)
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION