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

Fix casing in :title and add :slug for automatic downcasing in URLs #4100

Merged
merged 1 commit into from Nov 18, 2015

Conversation

rebornix
Copy link
Contributor

@rebornix rebornix commented Nov 3, 2015

The only difference between slug and title is that uppercase letters are not converted to their lowercase counterparts in slug. After adding this property, the final permalink of _posts/2015-10-27-Jekyll.md with permalink style /:year/:month/:day/:slug will be /2015/10/27/Jekyll/.

This pr is related to issue #4072 and it's my first time writing ruby, correct me if I made anything wrong.

@envygeeks
Copy link
Contributor

I'm on the fence with this, I don't know if the rest of @jekyll/core will be but surely to me a slug is the one that is normalized, so this is the flip of that. I would much rather slug to be something like "cased_title" or something to that effect.

@rebornix
Copy link
Contributor Author

rebornix commented Nov 4, 2015

Actually the word slug confuses me all the time, when we are trying to extract the title from file name (see code here),

m, cats, date, slug, ext = *relative_path.match(DATE_FILENAME_MATCHER)

we are naming the variable as slug and it's not normalized.

@envygeeks your suggestion is great, cased_title is more intuitive and accurate. I'll take your advice if others are Okay with this name.

@parkr
Copy link
Member

parkr commented Nov 4, 2015

What is most backwards-compatible? I thought :title was Cased and :slug was uncased.

@envygeeks
Copy link
Contributor

In theory if I were programming an app (a blog) that was in Rails or Lotus slug would be the one that downcases, normalizes and transforms and title would be the one that keeps case and does the minimal of normalizing to ensure that a path doesn't break.

@parkr
Copy link
Member

parkr commented Nov 4, 2015

What did Jekyll do before, though? That's what I'm asking. It sounds like 3.0 changed this behaviour, and I'm not sure it did it for any good reason other than my own laziness or bad testing.

@rebornix
Copy link
Contributor Author

rebornix commented Nov 4, 2015

title was Cased before 3.0, but after upgrade to 3.0, it became lowercase now. As I mentioned in #4072, any blog which contains posts like "2015-10-27-Cased.md" will break. It's a breaking change.

@parkr
Copy link
Member

parkr commented Nov 4, 2015

Ok so let's make :title cased and :slug uncased.

@rebornix
Copy link
Contributor Author

rebornix commented Nov 4, 2015

In old version Jekyll, we haven't moved url_placeholders from post.rb to document.rb, we assigned slug in post.rb to title

def url_placeholders
      {
        :year        => date.strftime("%Y"),
        :month       => date.strftime("%m"),
        :day         => date.strftime("%d"),
        :hour        => date.strftime("%H"),
        :minute      => date.strftime("%M"),
        :second      => date.strftime("%S"),
        :title       => slug,

while in post.rb, slug is just the title we extract from filename.

def process(name)
      m, cats, date, slug, ext = *name.match(MATCHER)
      self.date = Utils.parse_date(date, "Post '#{relative_path}' does not have a valid date in the filename.")
      self.slug = slug
      self.ext = ext
    end

But at that time, we don't even have slugify. So I suppose this issue is introduced by utils.slugify.

I'll update my pr ASAP.

@rebornix
Copy link
Contributor Author

rebornix commented Nov 5, 2015

I've updated my pr to make :title cased and add uncased :slug for user customization, pls help review 😄

@envygeeks
Copy link
Contributor

That's a pretty complete pull request. I'm 👍 but just one request, can you squash it?

@rebornix
Copy link
Contributor Author

rebornix commented Nov 5, 2015

@envygeeks it's done. Can't wait to see this commit gets merged.

@rebornix
Copy link
Contributor Author

rebornix commented Nov 6, 2015

@parkr does this PR look good to you?

@Sneezry
Copy link

Sneezry commented Nov 9, 2015

Hey @envygeeks @parkr

I have the same issue when I build my site with the latest version Jekyll 3.0, and all my links are broken, please merge this pr as soon as possible to solve my problem, thx.

Zhe Li

@@ -186,7 +186,8 @@ def url_placeholders
path: cleaned_relative_path,
output_ext: output_ext,
name: Utils.slugify(basename_without_ext),
title: Utils.slugify(data['slug']) || Utils.slugify(basename_without_ext),
title: Utils.slugify_cased(data['slug']) || Utils.slugify_cased(basename_without_ext),
Copy link
Member

Choose a reason for hiding this comment

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

should this be slugify(data['slug'], cased: true) perchance?

Copy link
Member

Choose a reason for hiding this comment

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

we can change the api later, i guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parkr named parameter is a better solution, thanks for advice!

BTW, you may want to take a look at issue #4135 , users are complaining about their underscore are replaced by hyphen, maybe someday users would say they want their ~ back. Should we use Pretty mode here for :title, seems Pretty mode [^[:alnum:]._~!$&'()+,;=@]+ is designed to handle this kind of issue.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that sounds good.

@parkr parkr added the fix label Nov 16, 2015
@rebornix rebornix force-pushed the master branch 2 times, most recently from 4f59349 to fec9a37 Compare November 17, 2015 09:52
@parkr
Copy link
Member

parkr commented Nov 17, 2015

@parkr does this PR look good to you?

@rebornix Cucumber tests fail :(

@rebornix
Copy link
Contributor Author

@parkr such a silly mistake :( . Fixed now.

parkr added a commit that referenced this pull request Nov 18, 2015
@parkr parkr merged commit d7dc9d8 into jekyll:master Nov 18, 2015
parkr added a commit that referenced this pull request Nov 18, 2015
@Stargator
Copy link

👍 @rebornix For a first go at Ruby you did great! I'm learning Ruby as well.

@parkr
Copy link
Member

parkr commented Nov 18, 2015

^^ agreed!

@yous
Copy link
Contributor

yous commented Nov 22, 2015

Hi, I used my permalink setting as /:year/:month/:day/:title/. Then after upgrading Jekyll from 3.0.0 to 3.0.1, the links are changed, for example, from /2013/02/18/ios-6-1-music-album-shuffle/ to /2013/02/18/ios-6.1-music-album-shuffle/. Note the 6-1 and 6.1. For the consistency, I need to change the permalink setting to /:year/:month/:day/:slug/.

I agree to the direction of change, making title pretty. But I think this might be considered as a breaking change, since it can break some sites silently as URLs are delicate. I see this pull request as adding :slug and changing :title, not fixing it.

yous added a commit to yous/yous.be that referenced this pull request Nov 22, 2015
@rebornix
Copy link
Contributor Author

@yous Actually if you are upgrading Jekyll from 2.x to 3.0.1, you will not notice any breaking change. From my perspective, 3.0.1 is like a quick turnaround for some 3.0.0 unplanned breaking change.

BTW, instread of just making title pretty, I add slug for anyone who wants to keep up with Jekyll slugify trend. You got the chance to make everything correct, right?

People in community are trying to make slugify customizable, you might be interested.

@yous
Copy link
Contributor

yous commented Nov 23, 2015

@rebornix Oops, I hadn't noticed the change when I upgrade Jekyll 2.x to 3.0.0.pre.x. Well... it seems I need more tests. Thanks for pointing it out!

@envygeeks
Copy link
Contributor

If it's not noted in our upgrading docs that @parkr just released please file a ticket so we can update those docs to mention that.

yous added a commit to yous/yous.be that referenced this pull request Nov 24, 2015
@parkr parkr changed the title Add a new property slug to document url_placeholders. Fix casing in :title and add :slug for automatic downcasing in URLs Nov 25, 2015
jplitza added a commit to FreifunkBremen/bremen.freifunk.net that referenced this pull request Jan 8, 2016
Use :slug instead of :title in permalinks.

See jekyll/jekyll#4100
@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants