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

Update simple-icons names #3641

Closed
wants to merge 5 commits into from

Conversation

RedSparr0w
Copy link
Member

The main purpose of this PR is to avoid future issues like these:
#3639
#3297
#2980

This PR shouldn't break backwards compatibility,
This will just ignore any characters which are not a-z or 0-9 (case insensitive, even though they should already be lowercased),

for example let’s-encrypt will become letsencrypt.

@shields-ci
Copy link

shields-ci commented Jul 5, 2019

Warnings
⚠️

This PR modified helper functions in lib/ but not accompanying tests.
That's okay so long as it's refactoring existing code.

Messages
📖 ✨ Thanks for your contribution to Shields, @RedSparr0w!

Generated by 🚫 dangerJS against 8e3ae53

PyvesB
PyvesB previously approved these changes Jul 6, 2019
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

This looks good to me, it should prevent some users from making mistakes with tricky icon names. I agree that it shouldn't break backwards compatibility. 👍

@paulmelnikow
Copy link
Member

I'm frustrated there hasn't been any movement on the Simple Icons side of things (which is where I think this ought to be handled).

That said, it seems totally unreasonable to type that for Let's Encrypt or Macy's.

I'm reluctant though to support multiple different forms of these slugs indefinitely, which is what we'll have to do to avoid breaking changes going forward. Could we fix this in a slightly more conservative way by leaving the hyphen and period in there? So let’s-encrypt would become lets-encrypt, but we wouldn't start supporting letsencrypt, webcomponentsorg, or wolframlangauge.

Only accept the following characters and remove anything else:
a-z (case insensitive)
0-9
-
.
@RedSparr0w
Copy link
Member Author

RedSparr0w commented Jul 8, 2019

Updated to only leave -, ., a-z and 0-9 characters:

Input Output
let’s-encrypt lets-encrypt
.NET .net
Not a Re@l Service not-a-rel-service
Yahoo! yahoo

Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

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

Thanks!

@RedSparr0w
Copy link
Member Author

RedSparr0w commented Jul 10, 2019

Ughh, Just realised that this will cause problems with google and google+ .
I assume the google+ logo would overwrite the google logo, which is less than ideal.

What do you think is the best way to handle this?

I might open a PR in simple-icons to remove G+ as its no longer active?
Although i'm not sure if it will be removed or not.

@calebcartwright
Copy link
Member

I might open a PR in simple-icons to remove G+ as its no longer active?

Sounds reasonable to me, probably worth asking the simple-icons team anyway 👍

@RedSparr0w
Copy link
Member Author

I've opened a PR simple-icons/simple-icons#1513,
I'll wait until that is merged and we update before merging this PR.

I'll also add a check for duplicates so we don't encounter the same problem in the future.

@ericcornelissen
Copy link

I approved and merged the removal of Google+ from Simple Icons, and we will likely release a new version this weekend 👍

I'm frustrated there hasn't been any movement on the Simple Icons side of things (which is where I think this ought to be handled).

My apologies, I'm sorry to hear that. I agree that it should be solved in Simple Icons as it would benefit our NPM package as well. TBH I had kinda forgotten about simple-icons/simple-icons#1362 😅 I will try to look at it as soon as possible...


I also wanted to check in and ask if you considered using the code in our utility functionality (the titleToFilename function) for conversion of the name? If it helps I will try to make it available as a separate entity (not sure if it should be released on NPM or just be GitHub repository dependency 🤔) since that is useful for some of our side projects as well. Though I guess that no longer matters once simple-icons/simple-icons#1362 is addressed...

@paulmelnikow
Copy link
Member

Thanks @ericcornelissen! We really appreciate the service you provide. I didn't mean to complain, was thinking I would bump simple-icons/simple-icons#1362 with a reminder / what can I do to help comment once we had this sorted out.

@paulmelnikow
Copy link
Member

I also wanted to check in and ask if you considered using the code in our utility functionality (the titleToFilename function) for conversion of the name? If it helps I will try to make it available as a separate entity (not sure if it should be released on NPM or just be GitHub repository dependency 🤔) since that is useful for some of our side projects as well. Though I guess that no longer matters once simple-icons/simple-icons#1362 is addressed...

Just seeing this! Yes, I think we could have used that function! Probably we'd have wanted it in the npm package rather than copying and pasting it (I'd be concerned it'd fall out of date). I guess since simple-icons/simple-icons#1362 is moving it's moot.

And a heads up to Shields folks: @ericcornelissen has asked for our feedback in simple-icons/simple-icons#1531.

@ericcornelissen
Copy link

Just seeing this! Yes, I think we could have used that function! Probably we'd have wanted it in the npm package rather than copying and pasting it (I'd be concerned it'd fall out of date). I guess since simple-icons/simple-icons#1362 is moving it's moot.

True, I just offered it as in intermediate solution before simple-icons/simple-icons#1362 is available in our NPM package...

@paulmelnikow
Copy link
Member

Thanks! I'm thrilled y'all are tackling this and think we probably can wait a few more days.

@chris48s
Copy link
Member

chris48s commented Aug 9, 2019

As of simple-icons 1.12.0, icons now have a .slug property.

Related reading:
#3801
simple-icons/simple-icons#1520

This is nice because it means we've got a consistent slug on each object which is discoverable from the simpleicons website/repository/library. Adopting this would be easier for users because our slugs would match the ones used elsewhere.

The downside is that the slugs we've generated in the past are quite different for a lot of icons, so just switching from key.toLowerCase().replace(/ /g, '-') to simpleIcons[key].slug would break a lot of badges in the wild. This is a rough diff between our slugs and theirs:

--- oldslugs	2019-08-09 21:39:22.237364058 +0100
+++ newslugs	2019-08-09 21:40:35.632137692 +0100
@@ -3 +2,0 @@
-dev.to
@@ -6,3 +5 @@
-rollup.js
-webcomponents.org
-.net
+dot-net
@@ -10 +7 @@
-about.me
+about-dot-me
@@ -14,12 +11,12 @@
-adobe-acrobat-reader
-adobe-after-effects
-adobe-audition
-adobe-dreamweaver
-adobe-illustrator
-adobe-indesign
-adobe-lightroom-cc
-adobe-lightroom-classic
-adobe-photoshop
-adobe-premiere
-adobe-typekit
-adobe-xd
+adobeacrobatreader
+adobeaftereffects
+adobeaudition
+adobedreamweaver
+adobeillustrator
+adobeindesign
+adobelightroomcc
+adobelightroomclassic
+adobephotoshop
+adobepremiere
+adobetypekit
+adobexd
@@ -27 +24 @@
-allociné
+allocine
@@ -29 +26 @@
-amazon-aws
+amazonaws
@@ -31 +28 @@
-american-express
+americanexpress
@@ -35 +32 @@
-angular-universal
+angularuniversal
@@ -38 +35 @@
-apache-flink
+apacheflink
@@ -40,2 +37,2 @@
-apple-music
-apple-pay
+applemusic
+applepay
@@ -43,2 +40,2 @@
-arch-linux
-archive-of-our-own
+archlinux
+archiveofourown
@@ -49 +46 @@
-at&t
+at-and-t
@@ -56,2 +53,2 @@
-azure-devops
-azure-pipelines
+azuredevops
+azurepipelines
@@ -63 +60 @@
-bath-asu
+bathasu
@@ -66 +63 @@
-big-cartel
+bigcartel
@@ -79 +76 @@
-brand.ai
+brand-dot-ai
@@ -83 +80 @@
-buy-me-a-coffee
+buymeacoffee
@@ -86 +83 @@
-campaign-monitor
+campaignmonitor
@@ -88 +85 @@
-cash-app
+cashapp
@@ -96 +93 @@
-cirrus-ci
+cirrusci
@@ -105 +102 @@
-code-climate
+codeclimate
@@ -123,2 +120,2 @@
-adobe-creative-cloud
-creative-commons
+adobecreativecloud
+creativecommons
@@ -128,4 +125,4 @@
-css-wizardry
-common-workflow-language
-c++
-d3.js
+csswizardry
+commonworkflowlanguage
+cplusplus
+d3-dot-js
@@ -133 +130 @@
-dassault-systèmes
+dassaultsystemes
@@ -141 +138 @@
-designer-news
+designernews
@@ -143,0 +141 @@
+dev-dot-to
@@ -155 +153 @@
-draugiem.lv
+draugiem-dot-lv
@@ -166 +164 @@
-eclipse-ide
+eclipseide
@@ -168 +166 @@
-elastic-cloud
+elasticcloud
@@ -170 +168 @@
-elastic-stack
+elasticstack
@@ -174 +172 @@
-empire-kred
+empirekred
@@ -176 +174 @@
-epic-games
+epicgames
@@ -182 +180 @@
-event-store
+eventstore
@@ -185 +183 @@
-experts-exchange
+expertsexchange
@@ -194 +192 @@
-fido-alliance
+fidoalliance
@@ -209,2 +207,2 @@
-fur-affinity
-furry-network
+furaffinity
+furrynetwork
@@ -224,2 +222,2 @@
-gnu-social
-gog.com
+gnusocial
+gog-dot-com
@@ -230,12 +228,12 @@
-google-allo
-google-chrome
-google-cloud
-google-analytics
-google-drive
-google-hangouts
-google-hangouts-chat
-google-keep
-google-pay
-google-play
-google-podcasts
-gov.uk
+googleallo
+googlechrome
+googlecloud
+googleanalytics
+googledrive
+googlehangouts
+googlehangoutschat
+googlekeep
+googlepay
+googleplay
+googlepodcasts
+gov-dot-uk
@@ -259 +257 @@
-hatena-bookmark
+hatenabookmark
@@ -274 +272 @@
-humble-bundle
+humblebundle
@@ -287 +285 @@
-intellij-idea
+intellijidea
@@ -289 +287 @@
-internet-explorer
+internetexplorer
@@ -291 +289 @@
-itch.io
+itch-dot-io
@@ -311 +309 @@
-khan-academy
+khanacademy
@@ -325,2 +323,2 @@
-laravel-horizon
-last.fm
+laravelhorizon
+last-dot-fm
@@ -329 +327 @@
-let’s-encrypt
+letsencrypt
@@ -335 +333 @@
-line-webtoon
+linewebtoon
@@ -338 +336 @@
-linux-foundation
+linuxfoundation
@@ -343 +341 @@
-macy’s
+macys
@@ -347 +345 @@
-mail.ru
+mail-dot-ru
@@ -354 +352 @@
-material-design
+materialdesign
@@ -365 +363 @@
-micro.blog
+micro-dot-blog
@@ -368,10 +366,10 @@
-microsoft-access
-microsoft-azure
-microsoft-edge
-microsoft-excel
-microsoft-onedrive
-microsoft-onenote
-microsoft-outlook
-microsoft-powerpoint
-microsoft-word
-linux-mint
+microsoftaccess
+microsoftazure
+microsoftedge
+microsoftexcel
+microsoftonedrive
+microsoftonenote
+microsoftoutlook
+microsoftpowerpoint
+microsoftword
+linuxmint
@@ -384 +382 @@
-monkey-tie
+monkeytie
@@ -389,2 +387,2 @@
-mozilla-firefox
-mx-linux
+mozillafirefox
+mxlinux
@@ -397 +395 @@
-next.js
+next-dot-js
@@ -402,3 +400,3 @@
-nintendo-gamecube
-nintendo-switch
-node.js
+nintendogamecube
+nintendoswitch
+node-dot-js
@@ -409 +407 @@
-nuxt.js
+nuxt-dot-js
@@ -412 +410 @@
-octopus-deploy
+octopusdeploy
@@ -415 +413 @@
-open-access
+openaccess
@@ -433 +431 @@
-picarto.tv
+picarto-dot-tv
@@ -438 +436 @@
-pivotal-tracker
+pivotaltracker
@@ -440 +438 @@
-player.me
+player-dot-me
@@ -442,2 +440,2 @@
-playstation-3
-playstation-4
+playstation3
+playstation4
@@ -456,2 +454,2 @@
-product-hunt
-proto.io
+producthunt
+proto-dot-io
@@ -471 +469 @@
-raspberry-pi
+raspberrypi
@@ -473 +471 @@
-read-the-docs
+readthedocs
@@ -477 +475 @@
-red-hat
+redhat
@@ -483,0 +482 @@
+rollup-dot-js
@@ -493 +492 @@
-samsung-pay
+samsungpay
@@ -496 +495 @@
-sauce-labs
+saucelabs
@@ -500 +499 @@
-scrutinizer-ci
+scrutinizerci
@@ -505 +504 @@
-server-fault
+serverfault
@@ -509,2 +508,2 @@
-simple-icons
-sina-weibo
+simpleicons
+sinaweibo
@@ -518 +517 @@
-smashing-magazine
+smashingmagazine
@@ -534 +533 @@
-speaker-deck
+speakerdeck
@@ -540,2 +539,2 @@
-stack-exchange
-stack-overflow
+stackexchange
+stackoverflow
@@ -557 +556 @@
-sublime-text
+sublimetext
@@ -559 +558 @@
-super-user
+superuser
@@ -570,2 +569,2 @@
-tencent-qq
-tencent-weibo
+tencentqq
+tencentweibo
@@ -573,2 +572,2 @@
-the-mighty
-the-movie-database
+themighty
+themoviedatabase
@@ -576 +575 @@
-tik-tok
+tiktok
@@ -586 +585 @@
-travis-ci
+travisci
@@ -592 +591 @@
-turkish-airlines
+turkishairlines
@@ -607 +606 @@
-unreal-engine
+unrealengine
@@ -622 +621 @@
-visual-studio-code
+visualstudiocode
@@ -624 +623 @@
-vlc-media-player
+vlcmediaplayer
@@ -626 +625 @@
-vue.js
+vue-dot-js
@@ -629,0 +629 @@
+webcomponents-dot-org
@@ -633 +633 @@
-when-i-work
+wheniwork
@@ -635 +635 @@
-wii-u
+wiiu
@@ -641,2 +641,2 @@
-wolfram-language
-wolfram-mathematica
+wolframlanguage
+wolframmathematica
@@ -644 +644 @@
-wp-engine
+wpengine
@@ -651,2 +651,2 @@
-y-combinator
-yahoo!
+ycombinator
+yahoo

Anyone got any thoughts on where to go from here..

@paulmelnikow
Copy link
Member

I opened #4273 to continue the discussion.

It seems clear that we should use the slugs, temporarily handle the old names, and make some kind of deprecation plan.

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.

7 participants