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

chore: fix explore pills #19866

Merged
merged 5 commits into from
Apr 28, 2022
Merged

chore: fix explore pills #19866

merged 5 commits into from
Apr 28, 2022

Conversation

villebro
Copy link
Member

@villebro villebro commented Apr 27, 2022

SUMMARY

Multiple small cosmetic tweaks to Explore:

  • Use the alert color for the "Altered" pill instead of the current mustardy one
  • Add alert type to Label component. I didn't migrate the AlteredSliceTag to the new component yet as the dark colors are slightly off in the alert color (the outline is set to the dark color if the label has a click handler). Will do it in a follow up once the colors are sorted out.
  • Remove redundant tooltip from RowCount pill
  • Remove suffix prop from RowCount component for consistency and make rowCount prop optional
  • Use pluralized translation for RowCount: when row count is one, the result is "1 row", otherwise "2 rows" etc
  • Capitalize "cached" to "Cached" to be in line with "Altered" pill
  • Convert some tests from JS to TS and migrate from Enzyme to RTL.

AFTER

Changes after ("1 row"/"4 rows" (pluralized), same RowCount for Results and Chart container, no tooltip for RowCount pill, capitalized "Cached", yellow "Altered" pill)

exp-pill-after.mp4

The new Label storybook with alert type:

image

The new RowCount storybook with singular/plural entries and removed suffix:

image

BEFORE

Changes after ("1 rows"/"4 rows" (always pluralized), different RowCount for Results and Chart container, redundant tooltip for RowCount pill, uncapitalized "cached", mustardy "Altered" pill)

exp-pill-before.mp4

Previous storybooks:

image

image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@villebro
Copy link
Member Author

Ping @kasiazjc for design review!

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

codes, LGTM!

@michael-s-molina
Copy link
Member

I wonder if the text should be black for the yellow label (same as gray label). What do you think @kasiazjc?

@villebro
Copy link
Member Author

I wonder if the text should be black for the yellow label (same as gray label).

Here's what it would look like:

image

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Approving it as the remaining comments are not blockers for me. Thanks for the improvements @villebro!

@netlify
Copy link

netlify bot commented Apr 27, 2022

Deploy Preview for superset-storybook ready!

Name Link
🔨 Latest commit ee1e723
🔍 Latest deploy log https://app.netlify.com/sites/superset-storybook/deploys/62694371ca05ad0009d30bb7
😎 Deploy Preview https://deploy-preview-19866--superset-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #19866 (a57dfd1) into master (ad878b0) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #19866      +/-   ##
==========================================
+ Coverage   66.48%   66.52%   +0.04%     
==========================================
  Files        1713     1714       +1     
  Lines       64995    65032      +37     
  Branches     6698     6714      +16     
==========================================
+ Hits        43209    43260      +51     
+ Misses      20079    20064      -15     
- Partials     1707     1708       +1     
Flag Coverage Δ
javascript 51.25% <100.00%> (+0.09%) ⬆️

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

Impacted Files Coverage Δ
...rset-frontend/src/components/CachedLabel/index.tsx 50.00% <ø> (ø)
...-frontend/src/components/AlteredSliceTag/index.jsx 88.23% <100.00%> (+0.35%) ⬆️
superset-frontend/src/components/Label/index.tsx 100.00% <100.00%> (ø)
.../src/explore/components/DataTableControl/index.tsx 70.66% <100.00%> (-0.39%) ⬇️
...end/src/explore/components/RowCountLabel/index.tsx 100.00% <100.00%> (+11.11%) ⬆️
...-chart-controls/src/sections/advancedAnalytics.tsx 14.28% <0.00%> (-19.05%) ⬇️
...-ui-chart-controls/src/components/MetricOption.tsx 90.90% <0.00%> (-3.83%) ⬇️
...ntend/src/views/CRUD/annotation/AnnotationList.tsx 69.35% <0.00%> (ø)
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <0.00%> (ø)
...nts/controls/DateFilterControl/DateFilterLabel.tsx 41.41% <0.00%> (ø)
... and 8 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 ad878b0...a57dfd1. Read the comment docs.

@rusackas
Copy link
Member

We may have a new issue with WCAG contrast between the white/yellow (which is why things went mustard-y in the first place), but we have to pick our battles.

@villebro
Copy link
Member Author

We may have a new issue with WCAG contrast between the white/yellow (which is why things went mustard-y in the first place), but we have to pick our battles.

Here's that "white on the new yellow background" test that fails: https://webaim.org/resources/contrastchecker/?fcolor=FFFFFF&bcolor=FCC700

@michael-s-molina
Copy link
Member

michael-s-molina commented Apr 27, 2022

We may have a new issue with WCAG contrast between the white/yellow (which is why things went mustard-y in the first place), but we have to pick our battles.

I personally prefer the yellow/dark gray text version but I'm fine with either version 😉

@ghost
Copy link

ghost commented Apr 27, 2022

Let's go with the grey text so it passes the accessibility check.

@villebro
Copy link
Member Author

Let's go with the grey text so it passes the accessibility check.

Will update accordingly, thanks @jess-dillard !

@villebro
Copy link
Member Author

For transparency, here's the check based on the new foreground:

https://webaim.org/resources/contrastchecker/?fcolor=323232&bcolor=FCC700

@villebro villebro merged commit 3d2fec9 into apache:master Apr 28, 2022
@villebro villebro deleted the villebro/alert branch April 28, 2022 07:28
hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
* chore: fix explore pills

* fix tests

* address comments

* add test and remove redundant div

* switch to dark text
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* chore: fix explore pills

* fix tests

* address comments

* add test and remove redundant div

* switch to dark text
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 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 preset-io size/L 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants