jekyll / jekyll-archives

:books: Archive pages for your Jekyll tags and categories.
https://jekyll.github.io/jekyll-archives/
MIT License
438 stars 99 forks source link

Allow creating slugs for emoji characters. #129

Closed xlgmokha closed 4 years ago

xlgmokha commented 6 years ago

I would like to be able to generate archives for categories identified by an emoji character.

For example:

https://www.mokhan.ca/ls/💎/

The current version of this gem replaces the emoji character with a blank string:

irb(main):002:0> x = "💎"
=> "💎"
irb(main):003:0> Jekyll::Utils.slugify(x)
=> ""
irb(main):004:0> Jekyll::Utils.slugify(x, mode: nil)
=> ""
irb(main):005:0> Jekyll::Utils.slugify(x, mode: 'raw')
=> "💎"

This behaviour causes the generation of archives in the root of the category permalink. i.e /:category/ instead of /:category/💎/.

My proposed solution adds a new configuration named slug_mode that allows jekyll users to change how slugs are generated. The :raw slug mode preserves the emoji character instead of replacing it with a blank string.

ashmaroli commented 6 years ago

I'd like this to be a "configurable" option instead of forcing :pretty slugs on everyone.

jekyll-archives:
  slug_mode: pretty # Modes supported by your Jekyll version.
                    # default: `nil`
xlgmokha commented 6 years ago

I updated the PR to read in the config['slug_mode'] configuration as suggested. Is there anything else I can do?

DirtyF commented 6 years ago

@mokhan Could you state the problem you're trying to solve here? What is the primary use case?

xlgmokha commented 6 years ago

I apologise for not clearly stating my problem. I updated the description. I hope this helps.

pathawks commented 6 years ago

It might also be useful for slugify to raise a pretty loud warning if the input was not an empty string but the output is.

ashmaroli commented 6 years ago

It might also be useful for slugify to raise a pretty loud warning

@pathawks Would that be something for Jekyll Core to do instead..?

pathawks commented 6 years ago

Would that be something for Jekyll Core to do instead..?

Yes. Sorry, I was assuming this was a Jekyll Core issue.

ashmaroli commented 4 years ago

@mokhan Sorry for the delay with this. Are you still interested in this feature? Upon revisiting, I realize that your assumption of the logic behind the implementation here is flawed.

The acceptable slugify modes have always been strings:

SLUGIFY_MODES = %w(raw default pretty ascii latin).freeze

And your implementation here presumably works in the tests because :pretty is not an acceptable slugify mode and therefore Jekyll just returns the downcased input string:

def slugify(string, mode: nil, cased: false)
  mode ||= "default"
  return nil if string.nil?

  unless SLUGIFY_MODES.include?(mode)
    return cased ? string : string.downcase
  end

  ...
end

Therefore, I'll keep this PR open for a week more for you to revisit. (You are free to close this voluntarily, if you don't want to pursue this any more).

xlgmokha commented 4 years ago

Are you still interested in this feature?

Yes.

This is still a problem:

irb(main):001:0> x = "💎"
irb(main):002:0> Jekyll::Utils.slugify(x)
=> ""
irb(main):003:0> Jekyll::Utils.slugify(x, mode: 'pretty')
=> ""
irb(main):004:0> Jekyll::Utils.slugify(x, mode: 'raw')
=> "💎"
irb(main):005:0> Jekyll::VERSION
=> "3.8.6"

If people want to generate a unique slug, this MR allows them to inject a custom slug mode.

Upon revisiting, I realize that your assumption of the logic behind the implementation here is flawed.

The acceptable slugify modes have always been strings:

SLUGIFY_MODES = %w(raw default pretty ascii latin).freeze

And your implementation here presumably works in the tests because :pretty is not an acceptable slugify mode and therefore Jekyll just returns the downcased input string:

Agreed. I can remove the call to to_sym and improve the test.

xlgmokha commented 4 years ago

Back to you @ashmaroli. :ping_pong:

ashmaroli commented 4 years ago

Thank you @mokhan @jekyllbot: merge +minor