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.

Sign up for GitHub

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

Jump to bottom

style: improve "Datasource & Chart Type" <Label>s #10971

Merged
merged 3 commits into from Sep 28, 2020
Merged

style: improve "Datasource & Chart Type" <Label>s #10971

merged 3 commits into from Sep 28, 2020

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Sep 20, 2020 •
edited

after

Screen Shot 2020-09-20 at 12 20 03 PM

before

Screen Shot 2020-09-20 at 12 21 09 PM

storybook

Screen Shot 2020-09-20 at 12 17 14 PM

@ktmud
Copy link
Member

ktmud commented Sep 21, 2020 •
edited

Overall great improvements! But I'm a little worried that the interaction with DatasourceControl may have become a little more confusing. The gray pill looks the same like the clickable labels in other controls (Viz Type and Time Range) but is not actually clickable. Using the cog icon for a dropdown menu also seems a little anti-pattern. What do you think of keeping it as a button/label with a dropdown menu and limiting this PR to just changing the style?

@mistercrunch
Copy link
Member Author

mistercrunch commented Sep 21, 2020 •
edited

I totally agree about the dataset label, maybe the label should have a caret and be a dropdown button, but it seemed harder than it should be to do that. About the 3 vertical dots?

@junlincc
Copy link
Member

may I humbly ask UI changes can somehow go thru Product and Design? 🙏 I was planning to work on those labels with Design next sprint. we received some feedback from multiple users ... @mistercrunch

@mistercrunch
Copy link
Member Author

mistercrunch commented Sep 21, 2020 •
edited

@junlinc I tagged design-review for input! Happy to consider other approaches that address the underlying motives.

About the motives, I think the recently tweaked default is way too dark and "pops" way too much in this current state. The previous "white on a lighter gray" had usability issues, so I figure I'd go "black on light-gray with a light border" and it looks much better to me than before.

More generally, I'm not sure if the label-with-popover pattern for controls is the right approach. I'd be curious to see how it would look if we'd redesign it from scratch.

Open to suggestion & tweaks!

@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 21, 2020
@mistercrunch
Copy link
Member Author

mistercrunch commented Sep 23, 2020 •
edited

Screen Shot 2020-09-22 at 11 26 43 PM

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2020 •
edited

Codecov Report

Merging #10971 into master will decrease coverage by 4.67%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10971      +/-   ##
==========================================
- Coverage   65.79%   61.12%   -4.68%     
==========================================
  Files         816      816              
  Lines       38422    38419       -3     
  Branches     3621     3621              
==========================================
- Hits        25280    23483    -1797     
- Misses      13034    14750    +1716     
- Partials      108      186      +78
Flag Coverage Δ
#cypress ?
#javascript 61.77% <100.00%> (ø)
#python 60.74% <ø> (-0.67%) ⬇️

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

Impacted Files Coverage Δ
...src/explore/components/controls/VizTypeControl.jsx 76.92% <ø> (-15.39%) ⬇️
.../explore/components/controls/DatasourceControl.jsx 64.86% <100.00%> (-18.92%) ⬇️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
... and 175 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 d056e3d...d8d7b08. Read the comment docs.

@rusackas
Copy link
Member

There's another PR open where all the icons are moving over from the design system to the codebase. I'm trying to keep the names matching between Figma and the code, so there's a bit of overlap/misalignment with what's in this PR.

In this PR, you're adding a few icons, which are also in the other one, but with different names. Would it be OK to ask for rebasing/renaming when that other PR slides in, lest they both merge and we have duplicates?

Mapping (names):
cog -> gear
minus-circle -> minus
plus-circle -> plus

FWIW, the emerging standard for naming seems to be underscores in filenames (partly thanks to Figma) and dashes in the name values, as you have it.

@rusackas rusackas mentioned this pull request Sep 24, 2020
Merged
6 tasks
@rusackas
Copy link
Member

I also took some of your ideas and melded them into #11034 which uses your inverted/lightened default color, but treats the stroke/border idea differently, applying it to labels of any color, but only when they have an onClick handler. Then, at least, the border systemically means "clickable".

That said, this is probably a stopgap solution in the longer term, and many of these clickable labels should be replaced by Buttons, Dropdowns, etc.

@mistercrunch
Copy link
Member Author

closing in favor of #11034

@mistercrunch
Copy link
Member Author

Oh, reopening for things not covered in #11034

@mistercrunch mistercrunch reopened this Sep 25, 2020
@pull-request-size pull-request-size bot added size/M and removed size/L labels Sep 25, 2020
@mistercrunch
Copy link
Member Author

@rusackas - rebased!

@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 26, 2020
@mistercrunch mistercrunch changed the title style: improve default <Label> style: improve "Datasource & Chart Type" <Label>s Sep 26, 2020
@mistercrunch mistercrunch force-pushed the better_default_labels branch 2 times, most recently from e759ac8 to 91d85f5 Compare September 27, 2020 17:31
@mistercrunch mistercrunch merged commit 046bd02 into apache:master Sep 28, 2020
@mistercrunch mistercrunch deleted the better_default_labels branch September 28, 2020 15:32
@ktmud ktmud mentioned this pull request Sep 28, 2020
Closed
2 tasks
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch @rusackas @auxten
style: improve "Datasource & Chart Type" <Label>s (apache#10971)
ab2e9a5 
* style: improve default <Label>

* fix name-shifting icon

* adding some styles to remove the inner drop shadow from the 'more' button

Co-authored-by: Evan Rusackas <evan@preset.io>
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rusackas rusackas rusackas approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 0.38.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mistercrunch @ktmud @junlincc @codecov-commenter @rusackas