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

Add the FFmpeg icon #5537

Merged
merged 12 commits into from May 4, 2021
Merged

Add the FFmpeg icon #5537

merged 12 commits into from May 4, 2021

Conversation

wangwei1237
Copy link
Contributor

@wangwei1237 wangwei1237 commented Apr 25, 2021

ffmpeg

Issue: Fixes #5384
Alexa rank: 22,811

Checklist

  • I updated the JSON data in _data/simple-icons.json
  • I optimized the icon with SVGO or SVGOMG
  • The SVG viewbox is 0 0 24 24

Description

Add the FFmpeg icon for issue: #5384

@github-actions github-actions bot added the new icon Issues or pull requests for adding a new icon label Apr 25, 2021
Copy link
Member

@adamrusted adamrusted 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 contributing this @wangwei1237!
Before I get to reviewing the SVG itself, it looks like there's an issue with the centering of the icon (see https://github.com/simple-icons/simple-icons/pull/5537/checks?check_run_id=2430819867). Another issue that's appearing is that in the JSON you've labelled the icon as FFMpeg, whereas in the icon itself it is (correctly) labelled FFmpeg. Any chance you could update the JSON to match when you take a look at centering the icon?

@adamrusted
Copy link
Member

Thanks for the update @wangwei1237 - though looking at the source SVG, I'm wondering if we should try and maintain the 3D effect (see below, with a 0.35pt gap between the front 'face' and the rest of the shape). Could I get the opinion of some other @simple-icons/maintainers before you put any more work into it though?

ffmpeg (1)

@adamrusted adamrusted added in discussion There is an ongoing discussion that should be finished before we can continue and removed changes requested labels Apr 25, 2021
@PeterShaggyNoble
Copy link
Member

@adamrusted, while I agree that that does look better, their own monochrome version as seen in their website's logo and favicon is just a flattened version of the entire shape.

@adamrusted
Copy link
Member

adamrusted commented Apr 30, 2021

their own monochrome version as seen in their website's logo and favicon is just a flattened version of the entire shape.

@PeterShaggyNoble Does that still mean the version in the PR needs to be updated to the flattened version of the 3d icon, as opposed to just the front face?

Copy link
Member

@adamrusted adamrusted 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 the update @wangwei1237 - though it looks like you've stretched the icon to fill the box (see yours in red, and source in black here). You need to keep the aspect ratio the same, so with the width being the higher value. My path is 24px (W) by 22.8468px (H) - should pass the linter once it's optimized. Would you mind taking another look at this?

@adamrusted adamrusted added changes requested and removed in discussion There is an ongoing discussion that should be finished before we can continue labels May 2, 2021
Copy link
Member

@adamrusted adamrusted left a comment

Choose a reason for hiding this comment

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

Thanks @wangwei1237 - the icon matches up perfectly now!
One problem I'm spotting is that now you've introduced unnecessary paths to the final SVG. I was able to fix this on my end in Illustrator by 'releasing' the compound path and using the 'Unite' tool. Could you take a quick look at that to try and remove the unnecessary points?

Copy link
Member

@adamrusted adamrusted left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the update @wangwei1237!
My only other critique would be that there appear to be some unnecessary points in the middle of straight lines (see here), though as it's passing the linter - I'd be happy to merge this as-is if you can't remove them.

Copy link
Member

@adamrusted adamrusted left a comment

Choose a reason for hiding this comment

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

I'm still seeing some differences around the edge of most of the image (see here with PR in red, source in black) that may be a precision thing, but may also be easily fixed by moving the points to better match the original. Could you take a look at adjusting the image, and comparing against the original on your end @wangwei1237?

@wangwei1237
Copy link
Contributor Author

I'm still seeing some differences around the edge of most of the image (see here with PR in red, source in black) that may be a precision thing, but may also be easily fixed by moving the points to better match the original. Could you take a look at adjusting the image, and comparing against the original on your end @wangwei1237?

let me try.

@wangwei1237
Copy link
Contributor Author

wangwei1237 commented May 4, 2021

I'm still seeing some differences around the edge of most of the image (see here with PR in red, source in black) that may be a precision thing, but may also be easily fixed by moving the points to better match the original. Could you take a look at adjusting the image, and comparing against the original on your end @wangwei1237?

let me try.

I double-checked the latest version, and now all the issues raised before have been resolved. As a newcomer to svg, thank you(@adamrusted, @PeterShaggyNoble ) very much for help. I am sorry for the time you consumed by the mistakes I take. Thanks @adamrusted, @PeterShaggyNoble again.

Copy link
Member

@adamrusted adamrusted left a comment

Choose a reason for hiding this comment

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

I am sorry for the time you consumed by the mistakes I take. Thanks @adamrusted, @PeterShaggyNoble again.

No worries! That's what we're here for!
The icon looks to match up perfectly now! Thanks for your work (and patience) on this one @wangwei1237! Hope to see some more contributions from you in the future! 💪

@adamrusted adamrusted merged commit 06e1afc into simple-icons:develop May 4, 2021
ericcornelissen added a commit that referenced this pull request May 9, 2021
# New Icons

- Adafruit (#5592)
- Alpine.js (#5607)
- BookBub (#5435)
- FFmpeg (#5537)
- Google Fonts (#5207)
- IOTA (#5520)
- Lit (#5571)
- Odysee (#5577)
- ONNX (#5472)
- OpenWrt (#5576)
- Purism (#5462)
- Pusher (#5568)
- Telegraph (#5501)
- Thingiverse (#5591)
- UFC (#5569)

# Updated Icons

- Ford (#5564)
- Hack The Box (#5555)
- Hackaday (#5555)
- HackHands (#5555)
- Hackster (#5555)
- Handshake (#5555)
- Handshake (#5555)
- Harbor (#5555)
- Hashnode (#5555)
- Haskell (#5555)
- Hasura (#5555)
- Haxe (#5555)
- HBO (#5555)
- HCL (#5555)
- Headspace (#5555)
- HelpDesk (#5555)
- HERE (#5555)
- Heroku (#5555)
- Hexo (#5555)
- Hilton (#5555)
- HipChat (#5555)
- HockeyApp (#5555)
- Home Assistant (#5555)
- Home Assistant Community Store (#5555)
- HomeAdvisor (#5555)
- Homebrew (#5555)
- Hootsuite (#5555)
- Hoppscotch (#5555)
- Houzz (#5555)
- HP (#5555)
- Huawei (#5555)
- HubSpot (#5555)
- Hugo (#5555)
- Hulu (#5555)
- Humble Bundle (#5555)
- Hypothesis (#5555)
- Hyundai (#5555)
- ICON (#5519)
- Transport for Ireland (#5621)
- Transport for London (#5621)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new icon Issues or pull requests for adding a new icon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FFmpeg icon
3 participants