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

added Google Scholar logo #1677

Merged
merged 7 commits into from
Sep 29, 2019
Merged

added Google Scholar logo #1677

merged 7 commits into from
Sep 29, 2019

Conversation

awwsmm
Copy link
Contributor

@awwsmm awwsmm commented Sep 28, 2019

Issue: #1338

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

This is a Google subdomain so the hex value I picked is "Google blue".

Logo was vectorised from a very small png and could probably be optimised further.

Andrew added 5 commits September 28, 2019 13:25
removed trailing newline per Travis CI
again attempting to remove final newline which is causing Linter to reject PR
@awwsmm
Copy link
Contributor Author

awwsmm commented Sep 28, 2019

This PR is failing on the linter, which says there's a newline at the end of the SVG (there isn't) and on buggy code which I didn't write:

SyntaxError: Unterminated string constant
    at JS_Parse_Error.get (eval at <anonymous> (/home/travis/build/simple-icons/simple-icons/node_modules/uglify-js/tools/node.js:20:1), <anonymous>:71:23)
    at formatError (util.js:609:16)
    at formatValue (util.js:513:18)
    at inspect (util.js:293:10)
    at format (util.js:160:12)
    at Console.warn (console.js:145:21)
    at data.icons.forEach.icon (/home/travis/build/simple-icons/simple-icons/scripts/build-package.js:57:15)
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/home/travis/build/simple-icons/simple-icons/scripts/build-package.js:48:12)
    at Module._compile (module.js:653:30)
  message: 'Unterminated string constant',
  filename: '0',
  line: 4,
  col: 7,
  pos: 76 }

@ericcornelissen
Copy link
Contributor

First off: thanks a lot for the effort you're willing to put into this project. It really means a lot! Thanks a bunch 🎉


This PR is failing on the linter, which says there's a newline at the end of the SVG (there isn't) [...]

There is but GitHub is hiding it from us. If there really wasn't a final newline you would see an extra symbol in the code on GitHub. Removing it through the GitHub IDE will automatically add it (as will many editors). However, as I pointed out in #1324 we are fine with removing this requirement from the linter (though that is going to depend on what I'm explaining below).

[...] and on buggy code which I didn't write

It's also not code that we wrote per se, it is actually uglify-js doing something weird. Interestingly, it is caused by the newline! I tested it locally and removing the newline resolves the problem. The \r\n (that is your newline) at the end of the SVG file seems to be confusing uglify-js (and therefor possibly also JS interpreters!) when we pass the SVG file's content to it.

I guess we can resolve this by checking for newlines on our side and removing them before running uglify-js 🤔 If you want this PR to be merged quickly I recommend you to remove the newline yourself (remove it with notepad.exe - or something similar on other OSs - and check the diff. Again don't try to do this in the GitHub IDE, and if you use an editor locally consider enabling displaying whitespace), if not then we will look into updating the build in the near future and your PR will be merge-able after that 🙂

@ericcornelissen ericcornelissen added the new icon Issues or pull requests for adding a new icon label Sep 28, 2019
@awwsmm
Copy link
Contributor Author

awwsmm commented Sep 29, 2019

Thanks for your quick response @ericcornelissen. I manually removed the newline and committed. Let's hope it works this time.

Copy link
Contributor

@ericcornelissen ericcornelissen left a comment

Choose a reason for hiding this comment

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

The vectorization looks good and I agree with the metadata you chose. Could you resize the icon so that it fits exactly (horizontally) inside the viewbox (i.e. remove the space circles in the image below)?

viewbox preview

@awwsmm
Copy link
Contributor Author

awwsmm commented Sep 29, 2019

Fixed the scaling! Sorry about that.

Screenshot 2019-09-29 16 38 58

@ericcornelissen ericcornelissen merged commit 5764877 into simple-icons:develop Sep 29, 2019
@ericcornelissen
Copy link
Contributor

Thanks a lot for you contribution @awwsmm, it is very much appreciated! 🎉

Also, thanks for your feedback on the contribution process (also this one), we will do our best to improve the experience for future contributors 😄

Fixed the scaling! Sorry about that.

Not a problem at all, thanks for the quick fix 😉

birjj added a commit that referenced this pull request Oct 6, 2019
# New icons

- Terraform (#1674)
- Google Scholar (#1677)
- TensorFlow (#1683)
- Skillshare (#1686)
- Eleventy (#1698)
- GreenSock (#1691)
- PyTorch (#1689)
- Qgis (#1697)
- Xiaomi (#1687)
- MuseScore (#1685)
- Gentoo (#1688)
- McAfee (#1700)

# Updated icons
- Laravel (#1692)

# Miscellaneous

- Now available for PHP through packagist (#1611)
- SVG files may no have a final newline (#1682)
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.

None yet

2 participants