Add `u-svg-inline-bg` mixin for inline SVG background

anselmbradford commented 5 years ago

Additions

Adds .u-svg-inline-bg( @name ) mixin and associated plugin.

Removals

Remove cf-core dependency from cf-icons.
Removes @cf-icon-path variable.

Changes

Convert cf-tables to use SVG image instead of PNG for sort button.
Adds missing "font variables" entry from cf-typography TOC.

Testing

gulp build && gulp docs && bundle exec jekyll serve should pass and launch the docs.
Visit http://localhost:4000/components/cf-tables/#sortable-tables and compare to https://cfpb.github.io/capital-framework/components/cf-tables/#sortable-tables
Visit http://localhost:4000/components/cf-forms/#select-dropdown and compare to https://cfpb.github.io/capital-framework/components/cf-forms/#select-dropdown
Visit http://localhost:4000/components/cf-forms/#checkboxes-and-radio-inputs and compare to https://cfpb.github.io/capital-framework/components/cf-forms/#checkboxes-and-radio-inputs

Screenshots

Before:

After:

Notes

We may want to scrutinize whether packages other than cf-icons also do not depend on cf-core and remove those dependencies.

Scotchester commented 5 years ago

Buttons and Pagination use icons in buttons and Expendables use the up and down chevrons in the Show/Hide label.

anselmbradford commented 5 years ago

Buttons and Pagination use icons in buttons and Expendables use the up and down chevrons in the Show/Hide label.

So I wasn't thinking clearly here in respect to cf-buttons etc being installed in isolation, since there are no direct CSS dependencies within those packages, but rather depend on the convention of adding SVG markup within the component with the cf-icon-svg class. However, thinking about this, we have some inconsistencies:

Components that depend on cf-icon do so because by convention their markup should include an SVG icon. However, some packages, like cf-tables do not depend on cf-icons, but could include SVG icons in the cells of the table. If it's the developer's responsibility to include cf-icons if they have SVG icon content within cf-tables content area, would there also be an argument that it is the developer's responsibility to do so for all packages that include SVG icon content (so cf-buttons, cf-expandables, and cf-pagination)?

cf-tables uses a data URL to include a PNG for its chevron. First, this should be an SVG, and second, if this is an SVG, why couldn't all SVG icons be included this way and cf-icons only be used as a dependency for direct icons used outside of another component's pattern.
cf-forms does include SVG as a background-image, which requires making the SVG file available to the CSS, such as is done here in the docs, and requires the use of the @cf-icon-path. What if this was instead an inlined SVG image set on the background-image?

Proposal: What if we converted all icons used within CF components to be inlined SVG in the CSS as data URLs. Here's a commit of what that would look like https://github.com/cfpb/capital-framework/pull/938/commits/be50dbe4e8d3771b079fccf3c68fcd984dd78e6e If we wanted to reference the cf-icon source file, what if we used something similar to https://github.com/atlassian/less-plugin-inline-svg (or a simpler find and replace) in the build step to embed the SVG. The path in the source file would be the default backup, if the developer used a build process that didn't replace them. Thoughts?

sephcoster commented 5 years ago

Unsure if this would all work given LESS and how it handles variables, but if we go this route could we create the data:image/svg+xml;charset=UTF-8,<svg xmlns= strings as @ variables and call them that way to make it easier to reference?

End result might be something like: background: url ( @icon-down-embed )which would reference the embedded SVG.

anselmbradford commented 5 years ago

Okay I've totally re-worked this PR and rebased and force pushed, so throw out what you have and re-pull if checking this again.

I realized we could use a custom less plugin to run JavaScript at build-time to inject the source SVG files inline into the background-image property via a mixin, which this PR adds.

Icons are added two ways: in the markup + CSS styling, and purely via CSS as a background-image. Say all icons were delivered purely via CSS. This offers some interesting trade-offs to consider:

+/-The inline SVG moves from the markup to the CSS, making the markup smaller and the CSS larger.
- + This makes the dom-tree node count lower, possibly making time to first content quicker.
- + This makes the icons absent when CSS is not loaded, versus unstyled and huge (https://github.com/cfpb/capital-framework/issues/863)
- - Since cf-icons is delivered to all pages, all embedded SVGs get delivered to all pages. If this was ALL icons, that's 1.6MB (however, this may be cached after first load). This could possibly be addressed through further work in https://github.com/cfpb/cfgov-refresh/issues/253.
- The width of the icon becomes more difficult. As a background-image the width needs to be explicitly set, which varies by icon (icons have consistent height, and non-consistent widths).
+ The bewd svg_icon function could be eliminated if all icons were delivered via CSS.
- Styling the icons becomes more difficult, since they aren't included in the dom-tree any longer. This means CSS filters need to be used to colorize them. The source icons would need to be set to white instead of black for the brightness filter to work.

niqjohnson commented 5 years ago

@anselmbradford: @caheberer and I took a look at this, and we're good replacing the custom up/down filled triangles with our standard up/down chevrons in sortable tables. Turns out the reason we didn't go with the chevrons originally was because the old chevrons were thinner and didn't have enough contrast in table headers. The recent re-draw of the icons fixes that issue, so it makes sense to get rid of that custom triangle icon.

One issue we did notice is that the current chevron is sitting a little low in the cell, which is causing a couple of minor visual issues:

The chevron looks off-center vertically in the cell
Sortable cells have a slightly lower baseline than non-sortable cells

Can you vertically center the chevron with the cell's text? I did a quick mockup of that in Photoshop by pushing the chevron (both up and down states) up by 3px to get everything aligned (I added guides to the images to make it a little easier to eyeball things):

State	Without guides	With guides
Current (down)
Pushed up 3px (down)
Pushed up 3px (up)

Once that alignment is fixed, this change looks a-ok from a design and UX perspective. Thanks!

anselmbradford commented 5 years ago

@Scotchester @niqjohnson Ready for review again. Thanks!

niqjohnson commented 5 years ago

@anselmbradford: The new alignment in the sortable tables looks perfect to me—thanks. I took a quick look at the other changes, too, and everything looked the same locally as in production (as expected). So this looks 👍 from the design and UX sides.

cfpb / capital-framework