feat: use svg for checkbox component #10799

eschutho · 2020-09-04T21:23:07Z

SUMMARY

Replaces the use of font-awesome for checkboxes with the svg components that already exist. There's also the potential to add the existing "half-checked" svg icon to the checkbox component as a prop as a future improvement.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

There are some minor visual differences with the new component:

Proposed:

Existing:

TEST PLAN

A story and test were added, but it can be manually tested in the save dashboard modal as in the example.

ADDITIONAL INFORMATION

rusackas · 2020-09-04T23:56:35Z

superset-frontend/src/components/Checkbox/Checkbox.test.tsx

+ */
+
+import React from 'react';
+import { ReactWrapper, shallow } from 'enzyme';


Just FYI, this shallow will break if we use any Emotion theme vars in the component (which we probably will before long, e.g. to affect the color(s) of the SVG). the theming helpers one line below also include a "styledShallow" you can import to solve that.

Looks like the styledShallow won't work without the HOC from Emotion because of the dive after the shallow render, but I think it could be a good plan to add emotion to this anyway.

rusackas · 2020-09-04T23:59:19Z

superset-frontend/src/components/Checkbox/Checkbox.stories.jsx

+
+// eslint-disable-next-line no-unused-vars
+export const Component = _args => {
+  const [{ checked, style }, updateArgs] = useArgs();


Pretty cool, I didn't know this even existed, actually. Nice that it lets the components do the toggling.

I still like the idea of knobs, too, so you can control any given component from a common location. I wonder if there's a way that both can co-exist, e.g. this updateArgs toggles the checked prop and toggles the knob's checkbox.

FWICT storybook seems to be trying to move people to args instead of knobs, since args is part of original package instead of an addon: https://github.com/storybookjs/storybook/blob/next/addons/controls/README.md#knobs-to-custom-args
I put together a draft of the buttons as an arg/controls to see if we could migrate knobs to args, and it seems to work, but wdyt?

Here's a view of the checkbox and button as args/controls (ignore the knob stories for now):

Args/controls LGTM!

superset-frontend/src/components/Checkbox/Checkbox.stories.jsx

mistercrunch · 2020-09-05T23:46:57Z

Welcome to the repo and community @eschutho !

rusackas · 2020-09-08T19:03:09Z

superset-frontend/src/components/Checkbox/index.tsx

-        }}
-      />
+    <span
+      style={{ verticalAlign: 'top', ...style }}


We could probably use emotion to make this <span... a <Styles... tag, using Emotion, to address this alignment and the piecemeal ones added to the SVG files, like so:

const Styles=styled.span` vertical-align: top; ... svg { vertical-align: top; } `

rusackas

Little nits/comments that may or may not be worth addressing, but it looks like a great improvement!

codecov-commenter · 2020-09-09T19:51:47Z

Codecov Report

Merging #10799 into master will decrease coverage by 0.86%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10799      +/-   ##
==========================================
- Coverage   61.22%   60.36%   -0.87%     
==========================================
  Files         802      373     -429     
  Lines       37814    23875   -13939     
  Branches     3555        0    -3555     
==========================================
- Hits        23153    14412    -8741     
+ Misses      14475     9463    -5012     
+ Partials      186        0     -186

Flag	Coverage Δ
#javascript	`?`
#python	`60.36% <ø> (-0.65%)`	⬇️

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

Impacted Files	Coverage Δ
superset/db_engines/hive.py	`0.00% <0.00%> (-85.72%)`	⬇️
superset/db_engine_specs/hive.py	`53.90% <0.00%> (-30.08%)`	⬇️
superset/db_engine_specs/mysql.py	`79.16% <0.00%> (-12.50%)`	⬇️
superset/db_engine_specs/presto.py	`70.85% <0.00%> (-11.44%)`	⬇️
superset/examples/world_bank.py	`97.10% <0.00%> (-2.90%)`	⬇️
superset/examples/birth_names.py	`97.36% <0.00%> (-2.64%)`	⬇️
superset/views/database/mixins.py	`80.70% <0.00%> (-1.76%)`	⬇️
superset/models/core.py	`86.66% <0.00%> (-1.67%)`	⬇️
superset/views/core.py	`74.24% <0.00%> (-0.25%)`	⬇️
superset/connectors/sqla/models.py	`89.48% <0.00%> (-0.25%)`	⬇️
... and 432 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 f685825...b45d96a. Read the comment docs.

eschutho · 2020-09-09T20:04:24Z

@rusackas I updated the button story to use args/controls... it's about the same amount of code, but there are implicit args that you get for free. See what you think.

rusackas · 2020-09-14T18:03:19Z

@eschutho I think this one's ready to rock, but just needs a little touchup to pass CI. Sorry it sat here so long! One thing I see causing CI to fail is that styled should now be imported from @superset-ui/core (due to the big consolidation effort there)

eschutho · 2020-09-15T22:29:06Z

@rusackas my bad for not checking up on this after rebasing! Thanks for calling it out.

rusackas

LGTM! Thanks!

* use svg for checkbox component * add vertical align to svg * use emotion styling * update import to superset core Co-authored-by: Elizabeth Thompson <elizabeth@preset.io>

pull-request-size bot added the size/L label Sep 4, 2020

willbarrett requested review from rusackas and nytai September 4, 2020 22:29

rusackas reviewed Sep 4, 2020

View reviewed changes

superset-frontend/src/components/Checkbox/Checkbox.stories.jsx Show resolved Hide resolved

rusackas reviewed Sep 8, 2020

View reviewed changes

rusackas approved these changes Sep 8, 2020

View reviewed changes

eschutho marked this pull request as draft September 9, 2020 01:02

eschutho force-pushed the elizabeth/checkbox-story branch from 0be9a62 to b45d96a Compare September 9, 2020 19:46

eschutho force-pushed the elizabeth/checkbox-story branch from b45d96a to a305281 Compare September 9, 2020 19:55

eschutho requested a review from rusackas September 9, 2020 20:04

eschutho marked this pull request as ready for review September 9, 2020 21:00

eschutho force-pushed the elizabeth/checkbox-story branch from a305281 to 9174184 Compare September 11, 2020 15:59

eschutho added 4 commits September 15, 2020 13:23

use svg for checkbox component

884917b

add vertical align to svg

fda2608

use emotion styling

7c8d4c8

update import to superset core

70e86dc

eschutho force-pushed the elizabeth/checkbox-story branch from 9174184 to 70e86dc Compare September 15, 2020 21:56

eschutho changed the title ~~use svg for checkbox component~~ feat: use svg for checkbox component Sep 16, 2020

rusackas approved these changes Sep 16, 2020

View reviewed changes

rusackas merged commit d3e9c56 into apache:master Sep 16, 2020

rusackas deleted the elizabeth/checkbox-story branch September 16, 2020 19:03

nytai mentioned this pull request Sep 16, 2020

fix: lint failures #10919

Closed

6 tasks

junlincc mentioned this pull request Sep 21, 2020

style: fix checkbox color #10970

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use svg for checkbox component #10799

feat: use svg for checkbox component #10799

eschutho commented Sep 4, 2020 •

edited

rusackas Sep 4, 2020

eschutho Sep 9, 2020

rusackas Sep 4, 2020

eschutho Sep 9, 2020

rusackas Sep 9, 2020

mistercrunch commented Sep 5, 2020

rusackas Sep 8, 2020

rusackas left a comment

codecov-commenter commented Sep 9, 2020 •

edited

eschutho commented Sep 9, 2020 •

edited

rusackas commented Sep 14, 2020

eschutho commented Sep 15, 2020

rusackas left a comment

feat: use svg for checkbox component #10799

feat: use svg for checkbox component #10799

Conversation

eschutho commented Sep 4, 2020 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

rusackas Sep 4, 2020

Choose a reason for hiding this comment

eschutho Sep 9, 2020

Choose a reason for hiding this comment

rusackas Sep 4, 2020

Choose a reason for hiding this comment

eschutho Sep 9, 2020

Choose a reason for hiding this comment

rusackas Sep 9, 2020

Choose a reason for hiding this comment

mistercrunch commented Sep 5, 2020

rusackas Sep 8, 2020

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 9, 2020 • edited

Codecov Report

eschutho commented Sep 9, 2020 • edited

rusackas commented Sep 14, 2020

eschutho commented Sep 15, 2020

rusackas left a comment

Choose a reason for hiding this comment

eschutho commented Sep 4, 2020 •

edited

codecov-commenter commented Sep 9, 2020 •

edited

eschutho commented Sep 9, 2020 •

edited