-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Introduce Inline Snippets tag #2001
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
Conversation
karreiro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR, @dejmedus! Excellent stuff! I've left some minor comments, but it's really cool to see inline snippets working :) 🚀
bdf3e2a to
266e1ed
Compare
Maaarcocr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that we are allowing variables to be passed to render like we do for blocks in shopify themes, I think it would be wise to have a shared interface here that we can use in both places.
what does this mean practically? instead of checking for SnippetDrop, if the ruby object that is passed responds to a certain method (let's say render or maybe a less common name that may not be mistakenly implemented by other drops) then it will get called.
This way we can remove our monkey patching of render in storefronts, as having callable objects is now a real liquid feature and it has a shared interface we can use.
c0918c5 to
1133386
Compare
Thank you for the detailed explanation @Maaarcocr! I added a |
52ae826 to
fecdb2c
Compare
Inline snippets will reduce code duplication and improve the developer experience, eliminating the need for one-off snippet files
Previously, inline snippets syntax looked a bit
different, they:
- used strings as tag identifiers
- defined tag arguments {% snippet "input" |type| %}
This PR updates snippets to better reflect
the currently proposed syntax
Co-authored-by: Orlando Qiu <[email protected]>
Currently, snippet files identified by strings. This PR makes changes to render to allow for new inline snippets to use variables as identifiers instead
This commit updates the render method to share parts of the snippet and block rendering logic to enable inline snippets to support `with`, `for`, and `as` syntax
fecdb2c to
47288cc
Compare
karreiro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR, @dejmedus! I just left one minor question left, but overall, everything looks good to me :)
karreiro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @dejmedus! Excellent stuff 🚀
|
looks solid. one suggestion, if we do not have a test for this, can you please add a test for something like: to see what file and line number the render for rendering a non existing snippet has? in theory I would expect this to render something like: but what I care about is |
b02ece2 to
eab457f
Compare
karreiro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes, @dejmedus! Excellent catches, @Maaarcocr!
…sages in the render tag
|
Awesome feature! Do we know when this will be added to Shopify? :) |
| partial = template.to_partial | ||
| template_name = template.filename | ||
| context_variable_name = @alias_name || template.name | ||
| elsif @template_name_expr.is_a?(String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we should first check is_a?(String) since its the common/hot path
|
|
||
| if template.respond_to?(:to_partial) | ||
| partial = template.to_partial | ||
| template_name = template.filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to respond to name as well for duck typing
| private | ||
|
|
||
| def assign_score_of(snippet_drop) | ||
| snippet_drop.body.nodelist.sum { |node| node.to_s.bytesize } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this makes sense. Assigning a snippet drop shouldn't rely on parsed AST for this functionality.
This PR implements RFC#1916 and introduces support for the new inline snippets
{% snippet %}tag, which allows us to define reusable template components directly within Liquid filesTraditionally, snippets are stored as separate
.liquidfiles in asnippets/directory and rendered using the{% render %}tag. The{% snippet %}tag allows us to define and render inline snippets in the same file{% snippet input %} {% doc %} @param {string} type @param {string} value {% enddoc %} <input type="{{ type }}" value="{{ value }}" /> {% endsnippet %} {% render input, type: "text", value: "Hi!" %}