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

Add 'title' to collection URLs #2864

Merged
merged 11 commits into from
Sep 5, 2014

Conversation

kansaichris
Copy link

Spiritual successor to #2799.

This addresses @parkr's comment from #2799:

I'd like them to map to Liquid variables (i.e. page.name == :name, etc). Ideally, we'd have:

  1. name == File.basename(path, ".*")
  2. title == page.title in Liquid, sluggified

This also addresses @gjtorikian's comment from #2847:

Oh dang, I didn't know :title slugified.

Yeah, on documents would be grand.

Let me know what you think! ❤️

@parkr
Copy link
Member

parkr commented Sep 1, 2014

This looks great! I'd like to make the handling of slug creation a bit simpler and remove it from the url_placeholders method. Maybe sluggify(name) and sluggify(title) || sluggify(name)?

@parkr
Copy link
Member

parkr commented Sep 1, 2014

Thanks for doing the actual coding here though, it's very appreciated. ❤️

@gjtorikian
Copy link
Member

Brilliant. 🎉

nil
else
name.downcase.gsub(/[^\w]/, " ").strip.gsub(/\s+/, '-')
end
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about this simplification?

if name
  name.downcase.gsub # ...
end

Or you can use unless name.nil?. IIRC, methods return nil by default. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Also can we use something else besides name? It's pseudo-reserved I think.

Copy link
Member

Choose a reason for hiding this comment

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

I'm beginning to think that a sluggify function would turn out to be immensely useful throughout the rest of the project as well, and since it doesn't really have anything to do with Documents in the first place, would it be better to move it to Utils?

Copy link
Member

Choose a reason for hiding this comment

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

I also think it should be called slugify, FWIW.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I suggest just stealing Rails method. \w is clever, but for truly "pretty" URLs it should be both lowercase and accepting of - or _.

Copy link
Member

Choose a reason for hiding this comment

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

👍 for moving it to Utils and for yoinking it from Rails.

Copy link
Author

Choose a reason for hiding this comment

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

I agree 💯% and will update this pull request accordingly. 😄

Incidentally, wouldn't it also be useful to expose the slugify method in a Liquid template? If so, should that wait for a separate pull request?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

If you want to add it to Jekyll::Filters, go ahead 😄 I think that would be a valuable add. It can just pass to Jekyll::Utils.slugify.

@parkr
Copy link
Member

parkr commented Sep 1, 2014

Looking gooooooood.

@kansaichris
Copy link
Author

Thanks for all the ✨ feedback, everyone! Let me know if you'd like me to make any more changes—I'd be particularly interested in suggestions for further unit tests. 😄

@kansaichris
Copy link
Author

@parkr Does this look good to merge?

@parkr parkr self-assigned this Sep 4, 2014
@parkr
Copy link
Member

parkr commented Sep 4, 2014

I'll take a look tonight or (more realistically) in the morning tomorrow. Sorry for the delay!

Thanks for all your contributions to Jekyll!!!!

@kansaichris
Copy link
Author

My pleasure! 😄 Just wanted to make sure that I didn't forget anything here.

# Replace each non-alphanumeric character sequence with a hyphen
slug = string.gsub(/[^a-z0-9]+/i, '-')
# Remove leading/trailing hyphen
slug.gsub!(/^\-|\-$/i, '')
Copy link
Member

Choose a reason for hiding this comment

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

is this ensuring that there are no double-hyphens? i.e. no "this is my !#-fave blogs" !~> "this-is-my--fave-blogs?

Copy link
Member

Choose a reason for hiding this comment

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

No, it ensures there's no starting or ending hyphen: "¡Hola!" !~> "hola".

Copy link
Author

Choose a reason for hiding this comment

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

What @gjtorikian said. 😄

Actually, the first regex ensures that there are no double hyphens:

string.gsub(/[^a-z0-9]+/i, '-')

Is this the desired behavior, or should we allow double hyphens in some situations?

Copy link
Member

Choose a reason for hiding this comment

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

I've never seen a double hyphen and though "That's a good use of a double hyphen in a URL!" 😸

Copy link
Member

Choose a reason for hiding this comment

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

No, it ensures there's no starting or ending hyphen

Merp, yes, I meant to refer to the entire method.

the first regex ensures that there are no double hyphens

Great! Then we're good to go.

@parkr
Copy link
Member

parkr commented Sep 4, 2014

LGTM excepting the above comment about double-hyphens.

@parkr parkr merged commit 58a7691 into jekyll:master Sep 5, 2014
parkr added a commit that referenced this pull request Sep 5, 2014
@parkr
Copy link
Member

parkr commented Sep 5, 2014

Thanks for all your hard work, @kansaichris! I think I'll try to wrap up a 2.4.0 release of Jekyll sometime this weekend or on Monday.

@gjtorikian
Copy link
Member

@parkr Humble ask if we can work on tagging #2870 into 2.4.0 too? We're working on some internal shtuff that depends on these two pieces. Would prefer to talk offline because of sensitive internally stuff about this. 😀

@kansaichris kansaichris deleted the add-title-to-collection-urls branch September 5, 2014 22:01
@kansaichris
Copy link
Author

🎉

parkr added a commit that referenced this pull request Jan 30, 2015
This mimicks posts most closely. It can be overridden by the
YAML front matter.

Undoes some of #2864.
@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

6 participants