Skip to content

Conversation

@EricFromCanada
Copy link
Contributor

@EricFromCanada EricFromCanada commented Feb 22, 2021

This PR updates the Liquid documentation for v5.0.0:

  • add documentation for new liquid, echo, and render tags in a new Templates page with comment, raw, and include
  • add documentation for currently-undocumented features that appear only in unit tests
  • add a version badge beside recently-added tags
  • pull some wording and sample changes from Shopify's Liquid docs
  • revise links, wording, examples & such for readability or completeness

Some of the sample code relies on this rouge PR to be highlighted correctly, which has yet to be merged and included in GitHub Pages.

My changes can be previewed here: https://ericfromcanada.github.io/liquid/

One more suggestion that I haven't implemented here is grouping the filters into "Array", "String", "Math", and "Other" categories, as is done in the Shopify docs. Thoughts? @shopmike @adamhollett

@tobi
Copy link
Member

tobi commented Feb 25, 2021

We have to adjust the docs to push people from {% comment %} tag to the new comment syntax before 5.0

redirect_from:
- /tags/comment/
- /tags/raw/
---
Copy link
Contributor

@ADTC ADTC Mar 27, 2021

Choose a reason for hiding this comment

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

Awesome! You did what I wanted in #1409 (comment) 😄

@dylanahsmith look at this! 🙂 (https://ericfromcanada.github.io/liquid/tags/template/)

Copy link
Contributor

@ADTC ADTC left a comment

Choose a reason for hiding this comment

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

@ADTC
Copy link
Contributor

ADTC commented Mar 27, 2021

One more suggestion that I haven't implemented here is grouping the filters into "Array", "String", "Math", and "Other" categories, as is done in the Shopify docs.

@EricFromCanada I have the same suggestion in #1409 and in general we have a positive agreement on it, though some concerns were raised in the issue. Take a look and please do it the best way you think you can! Seems like you already know your way around the docs source code anyway. 😄

The version badge is a nice touch, btw. 👍

Update: I implemented it in fc10dfb and you can see it at https://adtc.github.io/liquid/

image

@ADTC
Copy link
Contributor

ADTC commented Apr 1, 2021

Wow, things are slow this week! 🐰🥚🔍

Just wondering, what will be the timeline for accepting this PR? I had built up my own changes on top of this, which are yet to be added to the PR by @EricFromCanada.

If this PR is accepted, I can just open another PR here for my changes. Thanks for your attention! ❤️

PS: @tobi hi, if you're waiting for the new version of commenting system to be released, I'd suggest it's better to update the docs with the old version first, and later come back and add the new version in when the old version is released. Anyway we have version badges and deprecation badges in the new docs format if you ever need to use them.

Copy link
Contributor

@adamhollett adamhollett left a comment

Choose a reason for hiding this comment

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

Content changes look great. I wasn't able to check every changed link so the rendered version could benefit from some further tophatting. Thanks @EricFromCanada for your contributions!

Sounds like we need to document the new comment syntax, and that these changes depend on some external stuff so please don't take this approval as permission to merge. Just wanted to give my two cents. Someone closer to the Liquid project should decide when to merge.

---

Concatenates two strings and returns the concatenated value.
Adds the specified string to the end of another string.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much more accessible, thank you 👍

@ADTC
Copy link
Contributor

ADTC commented Apr 22, 2021

@adamhollett I opened a superseding PR #1427. I can integrate your suggestions there if you're willing to approve and merge it :)

@adamhollett
Copy link
Contributor

I opened a superseding PR #1427

Thank you for the contributions @ADTC. This PR is already quite substantial. Again, I'm not the one to decide when it should be merged but I'd personally like to see this one merged and your changes rebased on top of it to make both PRs smaller. That way we'll review each one individually.

@EricFromCanada EricFromCanada force-pushed the gh-pages branch 2 times, most recently from 6ca5b57 to f4a6868 Compare April 22, 2021 21:36
@ADTC
Copy link
Contributor

ADTC commented Apr 23, 2021

Thank you for the contributions @ADTC. This PR is already quite substantial. Again, I'm not the one to decide when it should be merged but I'd personally like to see this one merged and your changes rebased on top of it to make both PRs smaller. That way we'll review each one individually.

@adamhollett OK I understand. I was just wondering what's going on because I got complete silence for weeks on end about my own contributions and my comments until your comment above. Anyway I'll wait for this PR to be merged first. Hopefully that will be soon as I have a lot of interesting changes to rebase on it.

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.

7 participants