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.

Sign up for GitHub

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

Jump to bottom

Use svgo to format svg files #8408

Merged
merged 7 commits into from
Dec 19, 2018
Merged

Use svgo to format svg files #8408

merged 7 commits into from
Dec 19, 2018

Conversation

lipis
Copy link
Contributor

@lipis lipis commented Sep 21, 2018 •
edited
Loading

Just a test, we need to check thoroughly and maybe exclude/include more folders..

@DSchau
Copy link
Contributor

DSchau commented Sep 21, 2018

@lipis nice! This will be beneficial. I'll take a closer look at this in a few!

@lipis
Copy link
Contributor Author

lipis commented Sep 21, 2018

Eventually we can also add hooks so all the modified files will be formatted before commit (.js, .md, .yaml, .svg, etc)

lipis
lipis commented Sep 21, 2018
View reviewed changes
www/src/logo.svg Show resolved Hide resolved
@m-allanson
Merge branch 'master' into svgo
55f39a7 
* master: (1037 commits)
  Update starters.yml (gatsbyjs#10505)
  chore(release): Publish
  fix(graphql-skip-limit): fix hasNextPage (gatsbyjs#10504)
  chore: use cjs instead of esm for consistency (gatsbyjs#10494)
  feat(gatsby-remark-copy-linked-files): add support for video elements with `src` attribute (gatsbyjs#10395)
  typofix (gatsbyjs#10488)
  Add kobit.in to showcase (gatsbyjs#10496)
  fix(docs): window.reload => window.location.reload (gatsbyjs#10459)
  feat(www): add unbird feedback component to starter lib (gatsbyjs#10450)
  fix(blog): youfit case study typofix
  Doc improvements to Visual testing with Storybook guide (gatsbyjs#10436)
  fix(gatsby-plugin-offline): prevent incorrect revisioning of static file by workbox (gatsbyjs#10416)
  fix(starters): ttag repo link
  fix typo in pull request template (gatsbyjs#10454)
  fix(www) Fix query for plugin links always ?=undefined (gatsbyjs#10453)
  chore(release): Publish
  fix(gatsby): fix extracting StaticQuery nested in shorthand fragment (gatsbyjs#10443)
  fix(www): avoid querying for no-cache=1 (gatsbyjs#10389)
  fix(gatsby-image): update typescript definitions - properly mark fields as optional (gatsbyjs#10419)
  refactor(gatsby): improve EnsureResources (gatsbyjs#10224)
  ...
@m-allanson
Run svgo
25f132a
@lipis lipis requested a review from a team as a code owner December 17, 2018 17:17
m-allanson
m-allanson suggested changes Dec 17, 2018
View reviewed changes
Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

Thanks @lipis! I've merged the latest master into your branch and then re-run the svgo script.

Having a look through the re-optimised SVG's I'm seeing the following that need a closer look.

GitHub is reporting these as invalid, and can't display them:

  • www/src/assets/docs.svg
  • www/src/assets/showcase-feather.svg

Can these be fixed?

GitHub's rich diff is showing the before and after images as blank for these:

  • /examples/using-javascript-transforms/src/static/fonts/fontawesome/fontawesome-webfont.svg
  • /www/src/assets/ecosystem.svg
  • /www/src/assets/featured-sites-icons--white.svg
  • /www/src/fonts/Webfonts/futurapt_book_macroman/ftn45-webfont.svg
  • /www/src/fonts/Webfonts/futurapt_bookitalic_macroman/ftn46-webfont.svg
  • /www/src/fonts/Webfonts/futurapt_demi_macroman/ftn65-webfont.svg
  • /www/src/fonts/Webfonts/futurapt_demiitalic_macroman/ftn66-webfont.svg

I guess these will need to be checked manually by building the site and previewing them in-situ in the browser.

@lipis do you have the time to review these? No worries if not, we can mark this up as help wanted to see if anyone else can take it on.

@m-allanson m-allanson added the help wanted Issue with a clear description that the community can help with. label Dec 17, 2018
@fk
Copy link
Contributor

fk commented Dec 18, 2018

Hm, about those webfont SVGs — IMO we should ignore those…
unless anyone wants to check on very legacy iOS if they still work.

It's really very legacy iOS that is affected by this if I remember (4.1 or something, half a decade ago), so I assume those fonts to be quite irrelevant, anyway.

@fk
Copy link
Contributor

fk commented Dec 18, 2018

Eh, we don't even include those SVG fonts (https://github.com/gatsbyjs/gatsby/blob/master/www/src/components/layout.js#L16-L20) anywhere, e.g. https://github.com/gatsbyjs/gatsby/blob/master/www/src/fonts/Webfonts/futurapt_book_macroman/stylesheet.css#L25-L33

@m-allanson
Copy link
Contributor

Thanks @fk! I've removed all the svg fonts. Here are before and afters of the remaining svgs. Before on the left, after on the right:

Ecosystem icon

screenshot 2018-12-19 at 15 17 26

Featured site icon

screenshot 2018-12-19 at 15 19 23

@fk
Copy link
Contributor

fk commented Dec 19, 2018

@m-allanson LGTM!

@m-allanson
Merge branch 'master' into svgo
190ae67 
* master: (42 commits)
  Run prettier to fix lint error (gatsbyjs#10558)
  a few style guide updates
  docs: implement adding forms stub (gatsbyjs#9397)
  New situations recently have introduced these ideas into the style guide (gatsbyjs#9894)
  Style guide update (gatsbyjs#10444)
  Add pullquote citation (gatsbyjs#10546)
  chore(release): Publish
  Upgrade cli dependency yurnalist (gatsbyjs#10469)
  docs(showcase): Update two tags (gatsbyjs#10541)
  chore(release): Publish
  fix(gatsby): bump minimum react-hot-loader version to get quick fix (gatsbyjs#10542)
  fix(www): fix starter redirects (gatsbyjs#10539)
  fix(gatsby): add Safari 10 support to default terser options (gatsbyjs#10536)
  feat(showcase): Add Crispin Porter Bogusky (gatsbyjs#10462)
  docs: add note about using client-side environment variables  (gatsbyjs#10492)
  feat(gatsby): don't watch files in static directory for production builds (gatsbyjs#5807)
  feat(gatsby-transformer-screenshot): Added fullpage option for puppeteer API call (gatsbyjs#10517)
  Lint (gatsbyjs#10535)
  feat(blog): Add a blog post about exporting a site from drupal to Gatsby (gatsbyjs#9453)
  Update sites.yml (gatsbyjs#10514)
  ...
@m-allanson m-allanson removed the help wanted Issue with a clear description that the community can help with. label Dec 19, 2018
m-allanson
m-allanson approved these changes Dec 19, 2018
View reviewed changes
Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @lipis 👍

@m-allanson m-allanson merged commit d1cf874 into gatsbyjs:master Dec 19, 2018
@lipis lipis deleted the svgo branch December 19, 2018 17:53
@lipis
Copy link
Contributor Author

lipis commented Dec 19, 2018

Nice! 🍻 🍾

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
@lipis @gpetrioli
Use svgo to format svg files (gatsbyjs#8408)
72cd3f6 
* Use svgo to format svg files

* Format files

* Run svgo

* Remove unused svg fonts

* Remove one more svg font
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fk fk fk left review comments

@m-allanson m-allanson m-allanson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lipis @DSchau @fk @m-allanson