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

Land Rover #3195

Merged
merged 11 commits into from Jun 11, 2020
Merged

Land Rover #3195

merged 11 commits into from Jun 11, 2020

Conversation

NovaGL
Copy link
Contributor

@NovaGL NovaGL commented Jun 10, 2020

Issue: Closes #3175

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

Extracted and cleaned logo from PDF #3175

simpleicons(2)

@ericcornelissen ericcornelissen added the new icon Issues or pull requests for adding a new icon label Jun 10, 2020
@PeterShaggyNoble
Copy link
Member

PeterShaggyNoble commented Jun 10, 2020

Thanks for taking this one on, @NovaGL 👍 I'll review the SVG in a bit but, for now, the build is failing because of the # in the hex in the JSON (we should really get around to automating the formatting of that value, it's such an easy one to overlook) and because of the inclusion of the fill attribute on the <path> tag.

Also, if you've used the PDF as the source, we should go with that for the source URL. Unless someone can find a page on one of their websites that links to it. Unfortunately the logos in their brochures are an even bigger mess, made up almost entirely of images - if it wasn't for that then we could have use that for the source.

UPDATE: Scratch that, I'm now getting the same certificate error @runxel was getting on that URL. We'll need to figure something else out 🤔

Copy link
Member

@PeterShaggyNoble PeterShaggyNoble left a comment

Choose a reason for hiding this comment

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

Wow, that really is a mess! Almost as bad as the Vauxhall one - fair play to you for taking it on 👍

Other than the changes mentioned above, I'd be happy to merge this in as it is; the SVG looks great to me. It didn't quite line up with the version I came up with for comparison purposes but I suspect I may have been a bit overzealous in the number of paths I removed - I pared it down to just the 2 ovals and the wordmark. Given that, the mess the source is and that the choice of colour wasn't 100% clear, we will need one of the other @simple-icons/maintainers to throw a second eye over it anyway to double-check everything.

@NovaGL
Copy link
Contributor Author

NovaGL commented Jun 11, 2020

Done those changes as requested and linked directly to a PDF, although any of these PDFs should do.

https://www.landrover.com.au/download-a-brochure/index.html

After you remove all the paths some of the PDFS actually have a monochrome logo as the base, just needed a little bit of fixing.

@PeterShaggyNoble
Copy link
Member

Thanks for that, @NovaGL I would suggest, though, https://www.landrover.com.au/download-a-brochure/index.html as the source URL.

Other than that, I'm happy to merge this in pending another review. Seeing as it's still inaccessible, here's the PDF @NovaGL extracted the icon from, so you can compare like with like:
landrover.zip.

@NovaGL
Copy link
Contributor Author

NovaGL commented Jun 11, 2020

No problem, I just was basing it off this

Also, if you've used the PDF as the source, we should go with that for the source URL

@PeterShaggyNoble
Copy link
Member

Ah, yes, sorry, I should have made that clearer: That suggestion was just for that particular PDF as we couldn't find a page anywhere that linked to it directly. Generally, though, we'd use the page a PDF was found on as the source URL.

Run svgo again to fix the overzealous whitespace consumption of svgomg.
Copy link
Member

@runxel runxel left a comment

Choose a reason for hiding this comment

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

Great work, thanks @NovaGL!
There are still some superfluous points in the ellipses, but thats totally okay for now.
(Disclosure: I ran svgo again to insert some whitespace back in, which svgomg sadly removes – which will make the svg unusable in Illustrator; shows up properly in browsers tho.)

@runxel runxel merged commit 6d1dbfe into simple-icons:develop Jun 11, 2020
ericcornelissen added a commit that referenced this pull request Jun 14, 2020
# New Icons

- Canonical (#3184)
- Der Spiegel (#3168)
- DHL (#3048)
- Land Rover (#3195)
- Openlayers (#3165)
- Prime (#3010)
- Quasar (#3144)

# Updated Icons

- Apache ECharts (#3166)
- Apache Pulsar (#3181)
- Fur Affinity (#2979)
- Jupyter (#3170)
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.

land rover
4 participants