Allow custom slugs

[ericcornelissen]

Issue: Fixes #2260
Alexa rank: n/a

Checklist

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

Description

This updates the .jsonlintschema, build script, and tests to allow for "custom" slugs. This entails:

• It will be possible to specify a "slug" in _data/simple-icons.json, and
• If present, this slug is used in place of the slug that would be automatically generated by the titleToFilename function.

As a result, in the case that two brand names map to the same automatic slug, one of them can specify a custom slug that will be used instead. As per the discussion in #2260, we require an underscore (_) in custom slugs to avoid collisions with automatically computed slugs which will not be/are not allowed to contain a slug (any existing slugs that contain an underscore will be renamed in v5, after which underscores will always be automatically removed when converting a title to a slug).

I'm not sure if we should even mention custom slugs in the Contributing Guidelines, as it doesn't apply to most contributions and may cause some contributors to include it when it's not necessary.

[ericcornelissen]

Some clarification regarding the changes currently in this Pull Request:

• scripts/build-package.js: the key for icons with custom slugs must be distinct from those without. To keep "backwards-compatibility" (even though our documentation doesn't specify it) with the NPM exported value being an object, the keys of anything without a custom slug is kept as it was, and those with a custom slug get their custom slug as a key instead.
• scripts/templates/index.js: the for-loop is split into two separate for-loops for the following reason: if it's a slug match, it is definitely a match; but if it's title match, it might actually be a slug matching a title (because we're being case insensitive). Hence, we need to be sure that the query is not trying to find a slug before we start looking at titles.
  Admittedly, this is bad for performance (although the worst-case performance hasn't changed) but to be honest the performance is already pretty bad. I think we're better of advocating people to search based on the slug already now (while title is still possible) and remove title-based search in v5.

[PeterShaggyNoble]

A quick, initial comment for now.

[PeterShaggyNoble]

Should this not be an if/else statement (assuming Liquid supports them), to avoid all those replacements if slug is present?

[ericcornelissen]

Liquid does support if/else statements and we can write it using one, this is just the lazy version (from the programmer's perspective): Liquid will first do all the replacements on the filename - for all icons - and if the icon has a slug specified then the filename is replaced by the slug. It's just wasting a bunch of CPU cycles for icons that have a custom slug.

I'd be happy to change it if you prefer 🙂

[PeterShaggyNoble]

Actually, given that we'll have switched over to the new site before we have enough slug entries for this to be a major issue, I don't think it's such a big deal if we don't update it.

[ericcornelissen]

That was my reasoning for this going with this easier but more obscure solution as well 😄

[PeterShaggyNoble]

the keys of anything without a custom slug is kept as it was, and those with a custom slug get their custom slug as a key instead.

Could we make this a breaking change in v5 so that, for consistency, the slug is the key for all icons?

Also, and I know it's a bit late in the day for this but I had an idea last night: Instead of adding the slug directly to simple-icons.json, how about we just add the "modifier" and use it to construct the slug for the file name & slug? To give an example, instead of adding "slug": "hive_blockchain" for #3008 we add "modifier": "Blockchain" and then construct the slug as titleToSlug(icon.title)+"_"+titleToSlug(icon.modifier). This might afford us a little better future proofing should we ever decide to change how the slugs should be constructed (e.g., using - as the separator instead of _). Would this make it more difficult when linting, though, to check that all slugs are unique?

[ericcornelissen]

the keys of anything without a custom slug is kept as it was, and those with a custom slug get their custom slug as a key instead.

Could we make this a breaking change in v5 so that, for consistency, the slug is the key for all icons?

I'm not sure if I follow. I'm not against using the slug as key for all icons but that is indeed a "breaking" change (arguably it is not as this usage is not in our documentation, it is just there to keep some backwards compatibility with earlier versions), implying we can't release this until v5. The current approach is not breaking and perfectly backwards compatible and arguably (bit of a stretch perhaps) also consistent as 1) all brand without custom slugs are there and 2) all brands with custom slugs are not there.

Also, and I know it's a bit late in the day for this but I had an idea last night: Instead of adding the slug directly to simple-icons.json, how about we just add the "modifier" and use it to construct the slug for the file name & slug? To give an example, instead of adding "slug": "hive_blockchain" for #3008 we add "modifier": "Blockchain" and then construct the slug as titleToSlug(icon.title)+"_"+titleToSlug(icon.modifier). This might afford us a little better future proofing should we ever decide to change how the slugs should be constructed (e.g., using - as the separator instead of _).

Interesting suggestion, it seems a bit opaque to me as "modifier" does not really tell you what it is doing and is meaningless on its own - in other words it is only ever useful to us and never to the end user. It is arguably more future proof as you said though. Personally, I'm not sure future proofing outweighs the "downsides" here. It should be fairly straightforward to updates custom slugs manually with a Regular Expression-based Find-and-Replace with the current approach.

Also, I would actually also prefer using - over _, but that depends on #4936.

Would this make it more difficult when linting, though, to check that all slugs are unique?

No, or at least not so difficult as not to do it.

[PeterShaggyNoble]

implying we can't release this until v5

I was suggesting we make that change separately in v5 so as not to block the merging of this one.

in other words it is only ever useful to us and never to the end user

True. The use of modifier as the key was only a suggestion, though, we may be able to come up with a better term that might make it more useful but, if not, then lets proceed with the slug entry instead - we've been long enough discussing it as it is! 😆

Also, I would actually also prefer using - over _, but that depends on #4936.

Me too, using _ for alternate versions of icons (e.g., localised names) makes better sense, I think. Seeing as #4936 is going to be a breaking change, though, let's proceed with _ here in the interests of getting it merged and resolving some long standing issues/PRs like that Hive one. We can always switch to - instead in v5.

[ericcornelissen]

I was suggesting we make that change separately in v5 so as not to block the merging of this one.

👍

Me too, using _ for alternate versions of icons (e.g., localised names) makes better sense, I think.

I actually really liked your suggestion of putting them in subfolders, in which case we can ignore that category entirely!

Seeing as #4936 is going to be a breaking change, though, let's proceed with _ here in the interests of getting it merged and resolving some long standing issues/PRs like that Hive one. We can always switch to - instead in v5.

Agreed 👍 Additionally, actually using it might give us more insight into the trade-offs of putting different kinds of restrictions on the format of the slug.

[mondeja]

Although I don't really like the _ solution, this will work, and as the implementation LGTM, feel free to merge this @ericcornelissen 👍 but, this is a breaking change, right? If so, should not be marked as is?

[PeterShaggyNoble]

@mondeja, I may be wrong here but I don't think this by itself breaks anything; it's #4920 that includes the breaking change.

[mondeja]

I may be wrong here but I don't think this by itself breaks anything

Oh yes, you're right!

[ericcornelissen]

Indeed this isn't breaking because we're not actually changing any slugs yet, just adding the option to.

Although I don't really like the _ solution

Again, if there are other suggestions I'd be happy to hear them 🙂

[PeterShaggyNoble]

Until #4936 is resolved, I think _ is our only current option.

[PeterShaggyNoble]

Merging this in based on @mondeja's approval above 🤞🏻 Thanks for finally getting this one resolved, @ericcornelissen 👍🏻