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

Allow creating slugs for emoji characters. #129

Merged
merged 7 commits into from
Apr 13, 2020
Merged

Allow creating slugs for emoji characters. #129

merged 7 commits into from
Apr 13, 2020

Conversation

xlgmokha
Copy link
Contributor

@xlgmokha xlgmokha commented Nov 5, 2018

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
Copy link
Member

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`

@DirtyF DirtyF requested a review from a team November 5, 2018 09:56
@xlgmokha xlgmokha changed the title Allow creating slugs for emoji characters. WIP Allow creating slugs for emoji characters. Nov 5, 2018
@xlgmokha xlgmokha changed the title WIP Allow creating slugs for emoji characters. Allow creating slugs for emoji characters. Nov 5, 2018
@xlgmokha
Copy link
Contributor Author

xlgmokha commented Nov 6, 2018

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

@DirtyF
Copy link
Member

DirtyF commented Nov 6, 2018

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

@xlgmokha
Copy link
Contributor Author

xlgmokha commented Nov 6, 2018

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

```irb
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: :pretty)
=> "💎"
```
@pathawks
Copy link
Member

pathawks commented Nov 6, 2018

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
Copy link
Member

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

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

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

👍 from me. Just a minor question regarding a test..

test/test_jekyll_archives.rb Show resolved Hide resolved
@pathawks
Copy link
Member

pathawks commented Nov 6, 2018

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

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

@ashmaroli ashmaroli self-requested a review April 12, 2020 07:38
@ashmaroli
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

Back to you @ashmaroli. 🏓

@ashmaroli
Copy link
Member

Thank you @mokhan
@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit e7e6fdf into jekyll:master Apr 13, 2020
jekyllbot added a commit that referenced this pull request Apr 13, 2020
@xlgmokha xlgmokha deleted the patch-1 branch April 13, 2020 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants