Skip to content

Liquefied link tag#6269

Merged
jekyllbot merged 4 commits into
jekyll:masterfrom
brainbeanapps:liquefied_link_tag
May 2, 2018
Merged

Liquefied link tag#6269
jekyllbot merged 4 commits into
jekyll:masterfrom
brainbeanapps:liquefied_link_tag

Conversation

@alexey-pelykh

@alexey-pelykh alexey-pelykh commented Aug 4, 2017

Copy link
Copy Markdown
Contributor

This would allow to use

{% assign path = 'some/variable/path' %}
{% assign ext = 'md' %}
{% link {{path}}.{{ext}} %}
in addition to

{% link some/hardcoded/path.md %}

@danpalmer

Copy link
Copy Markdown

Some feedback on this, I think this could be confusing and might not be the best choice for implementation.

The use-case here (please correct me if I'm wrong) is to enable linking to dynamic files. Currently, link can only take a hard-coded relative path, but we want to be able to pass something else in.

This implementation re-uses liquid templating, nested within the process of templating another file, in a way which I think could get confusing and be a maintenance burden in the future.

I'd suggest that instead of templating the data passed to link, we should allow looking it up as a variable name in the current context. If we did this, your example could be achieved in a similar way with the following code:

{% assign path = 'some/variable/path' %}
{% assign fullpath = path|append: ".md" %}
{% link fullpath %}

{% link some/hardcoded/path.md %}

This re-uses existing functionality in the templating system to achieve the string building, rather than introducing another layer of templating, and I think this will be simpler to document and maintain.

I'd be interested to hear what you and the maintainers think of these different solutions, and let me know if you want to do this, otherwise I might open a PR with an implementation.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

This implementation is based on #6261, which itself uses the same approach as include tag. Current one is more similar to jekyll-assets tags. Anyhow, writing one line instead of three seems more convenient, even if that requires extra depth of templating.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@danpalmer updated

@DirtyF DirtyF requested a review from a team September 25, 2017 13:13
@DirtyF

DirtyF commented Sep 25, 2017

Copy link
Copy Markdown
Member

@alexey-pelykh Could you document a use case for this? Thanks.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@DirtyF sure, to which page that should go?

@DirtyF

DirtyF commented Sep 25, 2017

Copy link
Copy Markdown
Member

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@DirtyF done

@DirtyF DirtyF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks !

@DirtyF DirtyF requested a review from a team September 25, 2017 15:04
@pathawks

pathawks commented May 2, 2018

Copy link
Copy Markdown
Member

Is this a duplicate of #6261? Seems like one or the other should be closed.

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@pathawks #6261 is super-simple, while this one is "as it should be"

@pathawks

pathawks commented May 2, 2018

Copy link
Copy Markdown
Member

@alexey-pelykh Are you saying that should be closed in favor of this?

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

Which-ever implementation suits project vision better! I prefer #6269 over #6261 - check the test cases, this one supports more sophisticated usage

@pathawks

pathawks commented May 2, 2018

Copy link
Copy Markdown
Member

Could somebody from @jekyll/windows kick AppVeyor? It seems to have choked.

Otherwise this LGTM 👍

@ashmaroli

ashmaroli commented May 2, 2018

Copy link
Copy Markdown
Member

Could somebody from @jekyll/windows kick AppVeyor? It seems to have choked.

AppVeyor builds cannot be restarted.. The best solution here is pushing the merge 'master' branch into.. commit.

@pathawks

pathawks commented May 2, 2018

Copy link
Copy Markdown
Member

Thanks for all the work you put into this. Sorry it took so long to get this merged, but this will be a great feature in Jekyll 4.0 👍🍻

@jekyllbot: :shipit: +minor

@jekyllbot jekyllbot merged commit b95151c into jekyll:master May 2, 2018
jekyllbot added a commit that referenced this pull request May 2, 2018
@alexey-pelykh alexey-pelykh deleted the liquefied_link_tag branch May 3, 2018 02:39
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
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.

8 participants