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

specify 'other' code in liquid codeblocks as markup #2919

Closed
wants to merge 2 commits into from

Conversation

JaKXz
Copy link
Collaborator

@JaKXz JaKXz commented May 26, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented May 26, 2021

JS File Size Changes (gzipped)

A total of 1 files have changed, with a combined diff of +32 B (+5.7%).

file master pull size diff % diff
components/prism-liquid.min.js 564 B 596 B +32 B +5.7%

Generated by 🚫 dangerJS against 208830e

@RunDevelopment
Copy link
Member

@JaKXz Could you please explain the issue this PR is solving and give an example?

@JaKXz
Copy link
Collaborator Author

JaKXz commented May 27, 2021

Hey @RunDevelopment -- sorry for the empty description, thanks for the reply.

For the purposes of syntax highlighting I think we're happy to think of the "other" elements in a liquid codeblock as markup. I stole this code from twig since using language-twig works relatively well (better than language-liquid) for liquid codeblocks, e.g.
image

but the same codeblock with language-liquid does not highlight the "other" parts (in this example, the CSS) properly:
image

Happy to discuss further, please let me know what you think!

@JaKXz JaKXz changed the title specify 'other' code in .liquid files as markup specify 'other' code in liquid codeblocks as markup May 27, 2021
@RunDevelopment
Copy link
Member

I see. In that case, Liquide deserves a bit of an overhaul.

Twig's approach with an other token can work but we have something better: Markup templating. With markup templating, we can even get template inside attributes, e.g. <a href="{{ url }}">link</a>.

Markup templating can be used like this:
components.json
@@ -731,6 +731,7 @@
                "liquid": {
                        "title": "Liquid",
+                       "require": "markup-templating",
                        "owner": "cinhtau"
                },
components/prism-liquid.js
@@ -1,4 +1,8 @@
 Prism.languages.liquid = {
+       'delimiter': {
+               pattern: /^\{(?:\{\{|[%\{])-?|-?(?:\}\}|[%\}])\}$/,
+               alias: 'punctuation'
+       },
        'keyword': /\b(?:comment|endcomment|if|elsif|else|endif|unless|endunless|for|endfor|case|endcase|when|in|break|assign|continue|limit|offset|range|reversed|raw|endraw|capture|endcapture|tablerow|endtablerow)\b/,
        'number': /\b0b[01]+\b|\b0x(?:\.[\da-fp-]+|[\da-f]+(?:\.[\da-fp-]+)?)\b|(?:\b\d+(?:\.\d*)?|\B\.\d+)(?:e[+-]?\d+)?[df]?/i, 
        'operator': {
@@ -10,3 +14,12 @@ Prism.languages.liquid = {
                lookbehind: true
        }
 };
+
+Prism.hooks.add('before-tokenize', function (env) {
+       var liquidPattern = /\{(?:%[\s\S]*?%|\{\{[\s\S]*?\}\}|\{[\s\S]*?\})\}/g;
+       Prism.languages['markup-templating'].buildPlaceholders(env, 'liquid', liquidPattern);
+});
+
+Prism.hooks.add('after-tokenize', function (env) {
+       Prism.languages['markup-templating'].tokenizePlaceholders(env, 'liquid');
+});

This works okay:

image

However, this is only a quick draft I did in literally 5 minutes and I have never worked with Liquid before. This still needs testing, testing, and then some more testing.

Would you like to do this?

@JaKXz
Copy link
Collaborator Author

JaKXz commented May 27, 2021

Whoa, that's pretty cool! I'd be happy to try that out and report back.

In the meantime, do you think we could merge this as a stopgap [and that would also give us a better control/baseline to compare against]? I'm not sure why CI is half-passing though... :\

@RunDevelopment
Copy link
Member

do you think we could merge this as a stopgap

Sorry but I'm inclined to say no. The reason being, it doesn't work. Liquid highlights all < and > as operators, so no full HTML tags can be matched by other. Here's the example I used above highlighted using this branch:

image

I'm not sure why CI is half-passing though

Lint and tests pass. Liquid is poorly tested rn, so it's not surprising that they pass. The reason the CI fails is that you forgot to update the minified files. Just run npm ci && npm run build and commit all changes to update the build.

And yes, we commit the build...

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

Successfully merging this pull request may close these issues.

2 participants