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 a tagsCollection object holding a display & url safe tag label #34

Merged
merged 2 commits into from
Sep 14, 2016

Conversation

jarodtaylor
Copy link
Collaborator

@jarodtaylor jarodtaylor commented Aug 27, 2016

There are times when I want to display a list of tags that link to their respective index. Currently, the tags and tagsUrlSafe arrays are separate. This tagsCollection array allows us to do something like the following:

{{#each tagsCollection as |tag| }}
    <a href="/articles/tags/{{tag.urlSlug}}">{{tag.display}}</a>
{{/each}}

Rather than commit a breaking change I figured it made sense to just add an additional property to the metadata.

@jarodtaylor jarodtaylor changed the title Add a tagCollection object holding a display & url safe tag label Add a tagsCollection object holding a display & url safe tag label Aug 27, 2016
@jarodtaylor
Copy link
Collaborator Author

I just realized that the README would need to be updated, as well.

@jarodtaylor jarodtaylor mentioned this pull request Aug 29, 2016
@maiertech
Copy link

I think your tagCollection should be named tags. Then tagsUrlSafe can be removed because URL safe tags are available as tag.urlSlug.

@jarodtaylor
Copy link
Collaborator Author

@mdotasia I could have done that, but wasn't sure how much of a change @hswolff wanted to introduce. This change wouldn't introduce any breaking changes, should someone want to update their version. If @hswolff is fine with me replacing his original tags with my new array of objects, I'd do it.

@maiertech maiertech mentioned this pull request Sep 6, 2016
@maiertech
Copy link

maiertech commented Sep 6, 2016

One more thought on this: it occurred to me that using the same layout partial to display all tags from the global tags or page tags from a page's tags is not possible because each uses a different data structure to store the tag name and urlSafe version. I think this is a problem and really intuitive. That's why in PR #37 I follow the global tags when creating the page tags.

I don't think it's pretty but it works. I think that aligning the two tags to ensure that accessing tag and urlSafe from layout partial is the same needs some more work. Maybe there is a better way to model the global tags.

@hswolff
Copy link
Collaborator

hswolff commented Sep 13, 2016

So this works for me as a non-major change bump, which is nice. I feel like it'd be possible to achieve this via something like {{#each tags as |tag key| }} but that definitely feels awkward.

Can you update the README.md and then I'll happily merge and release?

Also if you want to add a follow-on PR after this that is a breaking change, wherein we remove tagsUrlSafe and move tagCollection to just be tags then we can release that as a major version change as it changes the API but one I think is more robust.

Thanks!

@jarodtaylor
Copy link
Collaborator Author

@hswolff I've updated the README. I'll submit another PR, soon, with the more robust API (for next major release).

Thanks!

@hswolff hswolff merged commit 2065d2a into totocaster:master Sep 14, 2016
@hswolff
Copy link
Collaborator

hswolff commented Sep 14, 2016

Awesome, thank you!

Merged and published as 1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants