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

Add show metadata button back to the explore view #6911

Merged
merged 2 commits into from
Feb 19, 2019

Conversation

xtinec
Copy link
Contributor

@xtinec xtinec commented Feb 19, 2019

- Add the show metadta button, accidentally removed from PR apache#6046, back to the explore view
- Remove dead code that is no longer reachable from DataSourceModal.jsx.
@xtinec xtinec added !deprecated-label:bug Deprecated label - Use #bug instead minor-review v0.31 labels Feb 19, 2019
@codecov-io
Copy link

codecov-io commented Feb 19, 2019

Codecov Report

Merging #6911 into master will increase coverage by 0.02%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6911      +/-   ##
==========================================
+ Coverage   63.83%   63.86%   +0.02%     
==========================================
  Files         421      421              
  Lines       20447    20446       -1     
  Branches     2218     2219       +1     
==========================================
+ Hits        13053    13057       +4     
+ Misses       7262     7257       -5     
  Partials      132      132
Impacted Files Coverage Δ
superset/assets/src/datasource/DatasourceModal.jsx 56.41% <ø> (+1.64%) ⬆️
.../explore/components/controls/DatasourceControl.jsx 69.23% <71.42%> (+15.06%) ⬆️

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 30cd0e3...b073a10. Read the comment docs.

@mistercrunch
Copy link
Member

I removed it since I thought it wasn't really relevant anymore (superset of the information can be shown in the modal)

@xtinec
Copy link
Contributor Author

xtinec commented Feb 19, 2019

I removed it since I thought it wasn't really relevant anymore (superset of the information can be shown in the modal)

Interesting. @vylc @dorq to chime in. UX suggested adding this button back during the testing of 0.31 for the convenience it provides in the explore view. Worth discussing here whether we want this button for 0.31 and beyond.

@mistercrunch
Copy link
Member

Yeah I don't have a strong opinion one way or another.

@vylc
Copy link

vylc commented Feb 19, 2019

we should add back in--takes at least 2 clicks + scroll to view columns via modal vs. 1 click w/ '+' button

@mistercrunch
Copy link
Member

LGTM

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

LGTM

@xtinec xtinec merged commit f8cf0fb into apache:master Feb 19, 2019
xtinec added a commit that referenced this pull request Feb 19, 2019
* Add show metadata button back to the explore view

- Add the show metadta button, accidentally removed from PR #6046, back to the explore view
- Remove dead code that is no longer reachable from DataSourceModal.jsx.

* Adding additional code back to make the button function and remove more dead code.

(cherry picked from commit f8cf0fb)
@michellethomas
Copy link
Contributor

michellethomas commented Feb 19, 2019

hey, just noticed this got merged, I've been working on #6816 to add back the ability to change the datasource from explore, as part of this we wanted to put the icons under a dropdown so that we don't have so many icons next to datasource. The show metadata button doesn't really work super well under a dropdown, and I'm not sure if it would be confusing to have both a dropdown and an icon next to the datasource.

I'll resolve the conflicts and look into different options for the show metadata button along with the dropdown. The datasource section has been cluttered with icons in the past, if you would like to bring the show metadata button back, can you help with suggestions for how to fit it into a less cluttered datasource dropdown? @xtinec @mistercrunch

@vylc
Copy link

vylc commented Feb 19, 2019

Let's have design propose some suggestions? i think the only requirement for the use case of creating a chart is that there needs to be a frictionless way for a user to scan through the columns and decide what to put on the chart. It doesn't need to persist throughout the entire chart creation, so the collapse/expand model works pretty well. @elibrumbaugh

@michellethomas
Copy link
Contributor

@vylc What if we put the metadata info in a modal? This would also be 2 clicks to get to so it's similar to just using the datasource editor, but clearly listing "See datasource metadata" in a dropdown might make it feel easier to use.

I'm concerned that having an icon next to a dropdown button would be confusing, and the collapse / expand metadata doesn't fit into a dropdown. If we need to use collapse/expand for metadata, it seems like we may just need to go back to listing a number of icons next to each other (which doesn't seem ideal).

I'm not sure design has time to do another round of review soon to make it fit with this addition, so it would be good to discuss options on our own first.

@vylc
Copy link

vylc commented Feb 19, 2019

@michellethomas only issue with modal is that it blocks you from actually working on the chart, so you have to x it out in order to add fields, then enable again to view, whereas the expand/collapse allows you to scroll
hmmm...what does your dropdown look like right now?

@michellethomas
Copy link
Contributor

@vylc This is what the dropdown looks like: #6816 (comment)

@michellethomas
Copy link
Contributor

If we don't have much time for design iteration right now, I can add the change datasource back as an icon and save the design changes for whenever we want to clean up this section.

@vylc
Copy link

vylc commented Feb 19, 2019

@michellethomas dropdown looks good. i think it might be okay to still have the + next to it... designers probably shudder at this, but is at least intuitive

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 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 !deprecated-label:bug Deprecated label - Use #bug instead v0.31 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants