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

Remove dashes from archive tags #82

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

gemfarmer
Copy link

This PR fixes a problem that was causing some tag archives to be non-existent.

What this fixes

Basically, it creates a custom post_attr_hash method that removes dashes from a tag before assigning its value to the hash that site.tags returns. This way there is only one reference that is a valid url when it has dashes and there is no tag conflict.

This is related to #24

Why?

On our site, some dash-delimited tag archives were were inaccessible because they closely resembled tags without dashes in them. For instance, the user-centered design tag was being confused with the user centered design tag. If there was one post with each of the above tags, visiting /tags/user-centered-design/ would bring up the post that was first in the list of site.posts instead of posts associated with both tags.

@parkr
Copy link
Member

parkr commented Nov 9, 2016

/cc @jekyll/archives

@gemfarmer
Copy link
Author

I'll have time to review and address the build failures next week.

@alfredxing
Copy link
Member

This is an interesting one! 😜
The main issue is: in general, should we merge tags that are actually different, but produce the same slug through Utils.slugify? This would have ramifications also for case sensitivity (#24).

If we want to merge tags (i.e. have /user-centered-design list posts from both tags), we can just call Utils.slugify on each tag (so basically the same as you have done, but using slugify instead of just the gsub for the dash).

If we don't want to merge tags, I'm not sure if all-out removing dashes is the way to go... I can imagine cases where the dash would make a difference in terms of meaning (also, removing the dash in this way would change how it's displayed via {{ page.title }}). In this case, we can write our own slugify function to preserve case and/or spaces (or just not slug at all).

Thoughts, comments, discussion appreciated!

@gemfarmer
Copy link
Author

@alfredxing, I don't think I did a great job explaining the issue that we were having, so let me clarify.

Certain tags that differed only by if/when they were dash delimited, user-centered design vs user centered design, for instance, were being merged in certain instances by jekyll archives, and kept as distinct tags in others.

Instances where they were merged:

  • The tag-specific page that jekyll-archives creates. If there was a set of 28 posts with user-centered design and set with 1 post with user centered design, the /tags/user-centered-design/ url would only have one set of tags. :/

Instances where they were made distinct:

  • The generated list of tags that jekyll-archives creates

Dash delimiting all the tags isn't an ideal reasons you stated above:

I'm not sure if all-out removing dashes is the way to go... I can imagine cases where the dash would make a difference in terms of meaning (also, removing the dash in this way would change how it's displayed via {{ page.title }})

But, this seemed to be the only way I could quickly ensure that the tags weren't inconsistently merged. I'm guessing a better approach would be to make sure that any dash delimited tag is a distinct tag in site.tags!

FWIW we have since avoided this entirely simply by making sure that our tags don't overlap. If fixing this bug is a major hassle, maybe we could add documentation encouraging people to be stricter with their configuration. We are using a gem to manage Jekyll front matter that has helped a lot with this.

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.

3 participants