Skip to content
This repository has been archived by the owner on Jul 10, 2020. It is now read-only.

Add u-svg-inline-bg mixin for inline SVG background #938

Merged
merged 2 commits into from
Jul 23, 2019

Conversation

anselmbradford
Copy link
Member

@anselmbradford anselmbradford commented Jun 21, 2019

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

  1. gulp build && gulp docs && bundle exec jekyll serve should pass and launch the docs.
  2. Visit http://localhost:4000/components/cf-tables/#sortable-tables and compare to https://cfpb.github.io/capital-framework/components/cf-tables/#sortable-tables
  3. Visit http://localhost:4000/components/cf-forms/#select-dropdown and compare to https://cfpb.github.io/capital-framework/components/cf-forms/#select-dropdown
  4. 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:
Screen Shot 2019-07-09 at 11 22 34 AM

After:
Screen Shot 2019-07-09 at 11 32 08 AM

Notes

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

@Scotchester
Copy link
Contributor

Scotchester commented Jun 21, 2019 via email

@anselmbradford
Copy link
Member Author

anselmbradford commented Jun 21, 2019

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 be50dbe
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?

@anselmbradford anselmbradford changed the title Remove unneeded cf-icon dependencies Proposal: Remove need for internal cf-icon dependency Jun 21, 2019
@sephcoster
Copy link
Contributor

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 anselmbradford force-pushed the ans_remove_icon_deps branch 2 times, most recently from 14fa40a to aec76e0 Compare July 9, 2019 15:17
@anselmbradford anselmbradford changed the title Proposal: Remove need for internal cf-icon dependency Add u-svg-inline-bg mixin for inline SVG background Jul 9, 2019
@anselmbradford anselmbradford force-pushed the ans_remove_icon_deps branch 2 times, most recently from 7cfa3d1 to af9e083 Compare July 9, 2019 15:27
@anselmbradford
Copy link
Member Author

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.
  • - 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
Copy link
Member

@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) current current-guides
Pushed up 3px (down) new new-guides
Pushed up 3px (up) flipped flipped-guides

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

Copy link
Contributor

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

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

Works as advertised, but a few necessary and a few optional changes to request. Going to test now in cfgov-refresh.

packages/cf-icons/src/cf-icons.less Outdated Show resolved Hide resolved
packages/cf-tables/src/cf-tables.less Outdated Show resolved Hide resolved
packages/cf-icons/src/cf-icons-svg-inline.js Outdated Show resolved Hide resolved
packages/cf-icons/src/cf-icons-svg-inline.js Outdated Show resolved Hide resolved
packages/cf-icons/src/cf-icons-svg-inline.js Show resolved Hide resolved
packages/cf-forms/src/molecules/form-fields.less Outdated Show resolved Hide resolved
packages/cf-icons/src/cf-icons.less Outdated Show resolved Hide resolved
packages/cf-icons/src/cf-icons.less Outdated Show resolved Hide resolved
@anselmbradford
Copy link
Member Author

@Scotchester @niqjohnson Ready for review again. Thanks!

@niqjohnson
Copy link
Member

@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.

Copy link
Contributor

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

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

Looks good!

@anselmbradford anselmbradford merged commit 203c669 into master Jul 23, 2019
@anselmbradford anselmbradford deleted the ans_remove_icon_deps branch July 23, 2019 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants