-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
chore: fix explore pills #19866
Conversation
16dfe09
to
4a6f181
Compare
4a6f181
to
d62aa96
Compare
Ping @kasiazjc for design review! |
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.
codes, LGTM!
da7903d
to
3c4cbea
Compare
I wonder if the text should be black for the yellow label (same as gray label). What do you think @kasiazjc? |
3c4cbea
to
7772a8f
Compare
7772a8f
to
801100d
Compare
superset-frontend/src/explore/components/RowCountLabel/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/RowCountLabel/index.tsx
Outdated
Show resolved
Hide resolved
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.
Approving it as the remaining comments are not blockers for me. Thanks for the improvements @villebro!
✅ Deploy Preview for superset-storybook ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 |
I personally prefer the yellow/dark gray text version but I'm fine with either version 😉 |
Let's go with the grey text so it passes the accessibility check. |
Will update accordingly, thanks @jess-dillard ! |
8baa07e
to
a57dfd1
Compare
For transparency, here's the check based on the new foreground: https://webaim.org/resources/contrastchecker/?fcolor=323232&bcolor=FCC700 |
* chore: fix explore pills * fix tests * address comments * add test and remove redundant div * switch to dark text
* chore: fix explore pills * fix tests * address comments * add test and remove redundant div * switch to dark text
SUMMARY
Multiple small cosmetic tweaks to Explore:
alert
color for the "Altered" pill instead of the current mustardy onealert
type toLabel
component. I didn't migrate theAlteredSliceTag
to the new component yet as the dark colors are slightly off in thealert
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.RowCount
pillsuffix
prop fromRowCount
component for consistency and makerowCount
prop optionalRowCount
: when row count is one, the result is "1 row", otherwise "2 rows" etcAFTER
Changes after ("1 row"/"4 rows" (pluralized), same
RowCount
for Results and Chart container, no tooltip forRowCount
pill, capitalized "Cached", yellow "Altered" pill)exp-pill-after.mp4
The new
Label
storybook withalert
type:The new
RowCount
storybook with singular/plural entries and removedsuffix
:BEFORE
Changes after ("1 rows"/"4 rows" (always pluralized), different
RowCount
for Results and Chart container, redundant tooltip forRowCount
pill, uncapitalized "cached", mustardy "Altered" pill)exp-pill-before.mp4
Previous storybooks:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION