-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Cleanup existing liquid tags #9085
Cleanup existing liquid tags #9085
Conversation
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.
This looks like a solid refactor. ✅
@@ -2,7 +2,7 @@ class KatexTag < Liquid::Block | |||
PARTIAL = "liquids/katex".freeze | |||
KATEX_EXISTED = "katex_existed".freeze | |||
|
|||
def initialize(tag_name, markup, tokens) | |||
def initialize(_tag_name, markup, _parse_context) |
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 think that markup
could benefit from being written as _markup
here since it seems unused - thoughts? 😄
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.
@juliannatetreault Ah but it is being used here; it's just a little obfuscated since it is initialized by the gem itself
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.
Ohh, I see! Thanks for pointing that out.
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.
Everything looks nice and cleaned up! 🧽 ✨
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.
Nice!
a27f068
to
a8e85b4
Compare
What type of PR is this? (check all applicable)
Description
In #8917 and digging through the
liquid
gem source code, we found that the third argument passed toinitialize
in liquid tags isparse_context
, nottokens
.While updating these references, I also:
super
is called in every liquid tag.I intentionally skipped
PollTag
, I need to update more logic on that. That'll be a separate PR.Related Tickets & Documents
Closes #9024
QA Instructions, Screenshots, Recordings
Specs should be appropriate here since I'm not changing any logic or behavior. If you'd like, you can include a bunch of different liquid tags in an article locally to ensure they work 🎉 .
Added tests?
Added to documentation?