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

Fix: expand lime emoji fill viewbox #93

Merged
merged 3 commits into from
Jun 11, 2024
Merged

Conversation

Gabe-Mitnick
Copy link
Contributor

Fixes issue #87 by scaling up the lime to fill the viewbox.

  • Scaled up the SVG and tweaked the path to fix visible rounding errors
  • Optimized the SVG with SVGO and manually edited the SVG code to match the original
  • Exported PNG from Illustrator and ran optipng for lossless compression

Did I do this right? Is there a suggested way to do SVG optimization and PNG generation, or should I leave it to Github Actions? I couldn't find instructions for this in the contribution guidelines so I followed the PR description from #77 .

@Gabe-Mitnick
Copy link
Contributor Author

Is the svgo check failing because of a problem in the PR? The error log seems like there's a bug in the action

@jdecked
Copy link
Owner

jdecked commented Jun 11, 2024

Is the svgo check failing because of a problem in the PR? The error log seems like there's a bug in the action

Yeah, unfortunately using GitHub Actions to push to PRs created from forks is extremely annoying for security reasons. Similarly, I can't lint/optimize the SVGs on main because I have branch protection turned on, and I don't want to turn it off.

I'm going to bypass the check for now. I'll have to come up with a better way to have SVG optimization be automated. (Suggestions welcome from anyone reading this – this is the only larger project I've maintained that uses GitHub. May Phabricator RIP.)

@jdecked jdecked merged commit c82a400 into jdecked:main Jun 11, 2024
1 check failed
@jdecked jdecked linked an issue Jul 17, 2024 that may be closed by this pull request
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.

Lime doesn't fill entire viewbox
2 participants