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

[Checkbox] inline SVG background-image for icon #2709

Merged
merged 7 commits into from
Jul 23, 2018

Conversation

giladgray
Copy link
Contributor

  • Checkbox suffered greatly in the switch to SVG icons because its implementation no longer relied purely on CSS: React state was necessary to ensure the correct icon was rendered at the correct time.
    • This introduced a bunch of subtle bugs where the DOM and the state could get out of sync (due to an incorrect implementation of un/controlled state).
  • This PR brings us back to the CSS-only world by embedding the SVG images for small-tick and small-minus in the checkbox CSS directly.
    • This is functionally identical to the oild font usage content: $pt-icon-small-tick (in fact, that's the line that was replaced) so we no longer even need to support the font fallback!
    • As a result, I was able to remove from the React component the indicator logic and the checked state (which was only needed to render the icon) and all associated tests.
    • This amounts to reverting almost all of Checkbox renders SVG icons #2323.

What about the bytes?

Each icon adds ~300B to the total file size. Yes, bytes. This is not significant.

blueprint.css 290K ⇒ 291K

Gilad Gray added 3 commits July 20, 2018 17:26
this brings it back to CSS-only so we can rely on pseudo-states!
core take a dev dep on a library that enables this, and runs it thru svgo!
@blueprint-bot
Copy link

remove font icon support, add background-image layer instead of ::before

Preview: documentation | landing | table

@blueprint-bot
Copy link

DRY mixin

Preview: documentation | landing | table

@blueprint-bot
Copy link

bring back the ::before for :active state

Preview: documentation | landing | table

@blueprint-bot
Copy link

lint

Preview: documentation | landing | table

Copy link
Contributor

@themadcreator themadcreator left a comment

Choose a reason for hiding this comment

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

Minors comments otherwise approved

// Sass function to inline a UI Icon svg and change its path color:
// svg("16px/icon-name.svg", (path: (fill: $color)) )
'svg': inliner('../../resources/icons', {
// run through SVGO first
Copy link
Contributor

Choose a reason for hiding this comment

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

if we run this through SVGO, does it use all the default settings? Are those okay? sometimes SVGO gets a little overzealous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah default settings and i don't think there's a way to configure. seems sufficient for now.

if we care about the settings then we should add a script to ensure the tracked icon files are already optimized.

&::before {
// embed SVG icon image as backgroud-image above gradient.
// the SVG image content is inlined into the CSS, so use this sparingly.
background-image: svg("16px/#{$icon}.svg", (path: (fill: $white)));
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just parameterize whole path? we'll never want 32px icons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we only have 16px and 20px icons and this way you can access either. in this case we don't need the 20px since the forms are so simple.

export interface ICheckboxState {
checked: boolean;
// Checkbox adds support for uncontrolled indeterminate state
Copy link
Contributor

Choose a reason for hiding this comment

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

/** */ docs comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants