Visitar URL original
Use more modern SVG sprite compiler by thisisdano · Pull Request #4665 · uswds/uswds · GitHub
Skip to content

Use more modern SVG sprite compiler - #4665

Merged
thisisdano merged 7 commits into
developfrom
dw-update-svg-sprite-compile
Oct 6, 2022
Merged

thisisdano merged 7 commits into
developfrom
dw-update-svg-sprite-compile

Conversation

@thisisdano

Copy link
Copy Markdown
Contributor

This updates the SVG sprite compiler from gulp-svg-sprite to gulp-svgstore (https://www.npmjs.com/package/gulp-svgstore). This does not have a dependency on svg-sprite so it avoids issues with phantomjs which should fix compatibility issues with arm64 processors as outlined in uswds/uswds-compile#25.

I've also taken the opportunity to update our Sass compiler to sass-embedded (https://www.npmjs.com/package/sass-embedded)

This also removes some low and minor vulnerabilities in our devDependencies.

It should get a bit more thorough testing, but I have not noticed any icon regressions.

@thisisdano
thisisdano requested a review from mejiaj May 4, 2022 18:19
@mejiaj
mejiaj requested a review from amyleadem May 6, 2022 14:18
@mgwalker

mgwalker commented May 6, 2022 •

Copy link
Copy Markdown

Tested on an ARM64 arch node:16 docker container; the install and npm run build both worked. When I diff the files in dist/img between the ARM64 container and an x86_64 container, there are no differences, but I have to admit I'm not sure how to compare the sprite sheet to previous builds because it is different, but maybe not in meaningful ways.

Here's a symbol from the sprite sheet from the old dist (as used by Tock; USWDS@2.13.0):

<symbol viewBox="0 0 24 24" id="zoom_out_map" xmlns="http://www.w3.org/2000/svg">
  <path fill="none" d="M0 0h24v24H0z"/>
  <path d="M15 3l2.3 2.3-2.89 2.87 1.42 1.42L18.7 6.7 21 9V3h-6zM3 9l2.3-2.3 2.87 2.89 1.42-1.42L6.7 5.3 9 3H3v6zm6 12l-2.3-2.3 2.89-2.87-1.42-1.42L5.3 17.3 3 15v6h6zm12-6l-2.3 2.3-2.87-2.89-1.42 1.42 2.89 2.87L15 21h6v-6z"/>
</symbol>

And here's the same symbol from the current build:

<symbol id="zoom_out_map" viewBox="0 0 24 24">
  <g enable-background="new 0 0 24 24">
    <g>
      <rect fill="none" height="24" width="24"/>
    </g>
    <g>
      <g>
        <g>
          <path d="M15,3l2.3,2.3l-2.89,2.87l1.42,1.42L18.7,6.7L21,9V3H15z M3,9l2.3-2.3l2.87,2.89l1.42-1.42L6.7,5.3L9,3H3V9z M9,21 l-2.3-2.3l2.89-2.87l-1.42-1.42L5.3,17.3L3,15v6H9z M21,15l-2.3,2.3l-2.87-2.89l-1.42,1.42l2.89,2.87L15,21h6V15z"/>
        </g>
      </g>
    </g>
  </g>
</symbol>

The main path elements are the same (just using different formatting, but the result should be the same). Most of the rest looks like it accomplishes the same set of things, but I'm not familiar enough with SVG to say for sure if the <path fill="none" d="M0 0h24v24H0z" /> and the <rect fill="none" height="24" width="24" /> accomplish the same thing.

EDIT: I also checked it out on an M1 Mac and it works a treat there as well. 😄 Thanks @thisisdano

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Everything appears to compile and render as expected

@thisisdano thisisdano added the Hold label May 6, 2022
@thisisdano

Copy link
Copy Markdown
Contributor Author

I want to hold this one for a little more testing, but I agree that it looks good so far. Let's just dig into the compiler's options, test the icons at a few sizes with our usual classes, and prep this for release after 3.0.1

@brunerae brunerae removed the Hold label May 6, 2022
@brunerae brunerae added this to the uswds 3.2.0 milestone Sep 30, 2022
@thisisdano
thisisdano merged commit c2cbf14 into develop Oct 6, 2022
@thisisdano
thisisdano deleted the dw-update-svg-sprite-compile branch October 6, 2022 17:39
@amyleadem amyleadem mentioned this pull request Oct 6, 2022
2 tasks done
This was referenced Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants