Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Liquid if HTML tag name #141

Closed
charlespwd opened this issue Dec 8, 2022 · 7 comments
Closed

Liquid if HTML tag name #141

charlespwd opened this issue Dec 8, 2022 · 7 comments
Labels
bug Something isn't working

Comments

@charlespwd
Copy link
Collaborator

Unformatted source

<{%if swipe%}product-slider-component{% else %}div{% endif %}>
</{%if swipe%}product-slider-component{% else %}div{% endif %}>

Expected output

<{% if swipe %}product-slider-component{% else %}div{% endif %}>
</{% if swipe %}product-slider-component{% else %}div{% endif %}>

Actual output

LiquidHTMLParsingError
@charlespwd charlespwd added the bug Something isn't working label Dec 8, 2022
@charlespwd
Copy link
Collaborator Author

charlespwd commented Oct 23, 2023

Won't fix. No intention to do this since folks can also put attributes in that if statement.

@saiichihashimoto
Copy link

What's a way to accomplish this that is supported? (besides copy/pasting the entire element to put in both the if and else)

@charlespwd
Copy link
Collaborator Author

charlespwd commented Oct 25, 2023

Option 1.

{% if swipe %}
  <product-slider-component with props that only this gets>
{% else %}
  <div>
{% endif %}

{% # content... %}

{% if swipe %}
  </product-slider-component>
{% else %}
  </div>
{% endif %}

Option 2.

{% liquid
  if swipe
    assign wrapper = "product-slider-component"
    assign props = "..."
  else 
    assign wrapper = "div"
    assign props = ""
  endif
%}
<{{ wrapper }} {{ props }}>
  {% # content... %}
</{{ wrapper }}>

@saiichihashimoto
Copy link

For Option 1 I still receive a LiquidHTMLParsingError.

I'll try out Option 2

@charlespwd
Copy link
Collaborator Author

charlespwd commented Oct 26, 2023

For Option 1 I still receive a LiquidHTMLParsingError.

What version are you using? What's the exact error? Example code? Shouldn't be happening. If it is, that's a bug!

@saiichihashimoto
Copy link

  {%- if media.media_type == 'model' -%}
    <product-model class="deferred-media media media--transparent gradient global-media-settings no-js-hidden" style="padding-top: 100%" data-media-id="{{ media.id }}">
  {%- else -%}
    <deferred-media class="deferred-media gradient global-media-settings media no-js-hidden" style="padding-top: {{ 1 | divided_by: media.aspect_ratio | times: 100 }}%" data-media-id="{{ media.id }}">
  {%- endif -%}
  <button id="Deferred-Poster-Modal-{{ media.id }}" class="deferred-media__poster" type="button">
    <span class="deferred-media__poster-button motion-reduce">
      {%- if media.media_type == 'model' -%}
        <span class="visually-hidden">{{ 'products.product.media.play_model' | t }}</span>
        {%- render 'icon-3d-model' -%}
      {%- else -%}
        <span class="visually-hidden">{{ 'products.product.media.play_video' | t }}</span>
        {%- render 'icon-play' -%}
      {%- endif -%}
    </span>
    <img
      srcset="{% if media.preview_image.width >= 493 %}{{ media.preview_image | image_url: width: 493 }} 493w,{% endif %}
        {% if media.preview_image.width >= 600 %}{{ media.preview_image | image_url: width: 600 }} 600w,{% endif %}
        {% if media.preview_image.width >= 713 %}{{ media.preview_image | image_url: width: 713 }} 713w,{% endif %}
        {% if media.preview_image.width >= 823 %}{{ media.preview_image | image_url: width: 823 }} 823w,{% endif %}
        {% if media.preview_image.width >= 990 %}{{ media.preview_image | image_url: width: 990 }} 990w,{% endif %}
        {% if media.preview_image.width >= 1100 %}{{ media.preview_image | image_url: width: 1100 }} 1100w,{% endif %}
        {% if media.preview_image.width >= 1206 %}{{ media.preview_image | image_url: width: 1206 }} 1206w,{% endif %}
        {% if media.preview_image.width >= 1346 %}{{ media.preview_image | image_url: width: 1346 }} 1346w,{% endif %}
        {% if media.preview_image.width >= 1426 %}{{ media.preview_image | image_url: width: 1426 }} 1426w,{% endif %}
        {% if media.preview_image.width >= 1646 %}{{ media.preview_image | image_url: width: 1646 }} 1646w,{% endif %}
        {% if media.preview_image.width >= 1946 %}{{ media.preview_image | image_url: width: 1946 }} 1946w,{% endif %}
        {{ media.preview_image | image_url }} {{ media.preview_image.width }}w"
      src="{{ media | image_url: width: 1946 }}"
      sizes="(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 100 | times: media_width | round }}px, (min-width: 990px) calc({{ media_width | times: 100 }}vw - 10rem), (min-width: 750px) calc((100vw - 11.5rem) / 2), calc(100vw - 4rem)"
      {% unless lazy_load == false %}loading="lazy"{% endunless %}
      width="973"
      height="{{ 973 | divided_by: media.preview_image.aspect_ratio | ceil }}"
      alt="{{ media.preview_image.alt | escape }}"
    >
  </button>
  <template>
    {%- liquid
      case media.media_type
      when 'external_video'
        assign video_class = 'js-' | append: media.host
        if media.host == 'youtube'
          echo media | external_video_url: autoplay: true, loop: loop, playlist: media.external_id | external_video_tag: class: video_class, loading: "lazy"
        else
          echo media | external_video_url: autoplay: true, loop: loop | external_video_tag: class: video_class, loading: "lazy"
        endif
      when 'video'
        echo media | media_tag: image_size: "2048x", autoplay: true, loop: loop, controls: true, preload: "none"
      when 'model'
        echo media | media_tag: image_size: "2048x", toggleable: true
      endcase
    -%}
  </template>

  {%- if media.media_type == 'model' -%}
    </product-model>
    {%- if xr_button -%}
      <button
        class="button button--full-width product__xr-button"
        type="button"
        aria-label="{{ 'products.product.xr_button_label' | t }}"
        data-shopify-xr
        data-shopify-model3d-id="{{ media.id }}"
        data-shopify-title="title"
        data-shopify-xr-hidden
        >
        {% render 'icon-3d-model' %}
        {{ 'products.product.xr_button' | t }}
      </button>
    {%- endif -%}
  {%- else -%}
    </deferred-media>
  {%- endif -%}

Error:

✖ prettier --ignore-unknown --write --cache .:
[error] snippets/product-thumbnail.liquid: LiquidHTMLParsingError: Attempting to close HtmlElement 'product-model' before LiquidTag 'if' was closed
[error]   173 |   </template>
[error]   174 |
[error] > 175 |   {%- if media.media_type == 'model' -%}
[error]       |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error] > 176 |     </product-model>
[error]       | ^^^^^^^^^^^^^^^^^^^^^
[error]   177 |     {%- if xr_button -%}
[error]   178 |       <button
[error]   179 |         class="button button--full-width product__xr-button"

Version 1.3.1

@charlespwd
Copy link
Collaborator Author

charlespwd commented Oct 30, 2023

Ah OK so I see what's up. This is hitting the limits of the heuristic I chose. You see, we have to strike a balance between being helpful and telling folks there's a problem with their code and allowing for problems like this. It's hard for us to do statically, so the logic I went with was the following:

“dangling” open or close HTML tags are OK if and only if:

  • they are found inside a Liquid branch,
  • there are at most two of the same type (2 open or 2 close) and nothing else.

In the snippet you provided, you have a nested if statement and a dangling close in the first branch and a dangling close in the other. So you get the default “holup you are closing something that isn't open” error.

I might relax this rule, but I'm struggling to find the balance. Especially if your case was the opposite (an open without a close), then it would be really hard to tell if that was intentional or not.

e.g. this would be difficult to tell if it was intentional or not. I'd say probably not.

{% if cond %}
  <product-model>
    {% if xr_button %}
       ...
    {% endif %}
{% endif %}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants