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

Support {% render 'snippet', %} named argument completion #475

Open
1 task done
stijns96 opened this issue Aug 20, 2024 · 3 comments
Open
1 task done

Support {% render 'snippet', %} named argument completion #475

stijns96 opened this issue Aug 20, 2024 · 3 comments
Labels
area:theme-check enhancement New feature or request open for contributions theme bugs that won't be prioritized in the near future

Comments

@stijns96
Copy link
Contributor

stijns96 commented Aug 20, 2024

Is your feature request related to a problem? Please describe.
I always have to check the self written documentation in my snippets to see what props I can pass to my snippet.

Describe the solution you'd like
I was thinking about something like this.

{% prop source = object | required: true %}
{% prop width = number | required: true | default: 768 %}

{%- liquid
  prop source = object | required: true
  prop width = number | required: true | default: 768
-%}

I know this image now shows the source autocomplete from HTML, but this could be an autocomplete/suggestion from Liquid. It works a little bit like typescript, but then for liquid.

Image

At this point I'm not sure if this will work for liquid, but it's just a suggestion that I'd like to see.

Checklist

  • I have checked and made sure that the proposal adheres to this plugin's principles
@karreiro
Copy link
Contributor

👋 Hi @stijns96, thanks for the suggestion!

This is an excellent idea, and I'll take it back to the team for discussion. I'll update this issue as soon as we have any updates.

@karreiro karreiro added enhancement New feature or request area:theme-check open for contributions theme bugs that won't be prioritized in the near future labels Aug 26, 2024
@karreiro
Copy link
Contributor

Dawn snippets follow a strong convention of defining accepted elements in the first comment of the file. I believe we can take advantage of this in a JSdocs fashion, as this is a purely tooling-focused feature. Additionally, we can use the comments to provide inline documentation while folks type, similar to the assistance provided with objects.

{% prop source = object | required: true %}
{% prop width = number | required: true | default: 768 %}

{%- render 'test', so█ -%}
                     ^-- code completes with source
{% comment %}
  Renders the test snippet

  Accepts:
  - source: {object} Blog object
  - width: {number} Width of the page

  Usage:
  {% render 'test', source: 'the source', width: 800 %}
{% endcomment %}

{%- render 'test', so█ -%}
                     ^-- code completes with source

This issue relates to this one: #114. While we're more focused on the render tag, it's valuable to keep that in mind :)

@stijns96
Copy link
Contributor Author

stijns96 commented Aug 27, 2024

So if I understand it correctly, we don’t want to create a new variable like prop, but we want to use the first comment for this? Although I like this technique, I think it comes with some limitations or we have to come op with a slightly different approach.

The limitation I see is that we can not easily add more requirements to the properties we want to pass. This idea actually came up when I was working on a Vue project. See their props documentation here.

I ones created an extended docs for a quite extensive image snippet.

{%- comment -%}
  Files must meet the following requirements:
    https://help.shopify.com/en/manual/shopify-admin/productivity-tools/file-uploads

  # Parameters:       | Required    | Type                | Description                               | Accepted values
    - source          | true        | { Object }          | The image to be displayed.                |
    - class           | false       | { String }          | The class for the image.                  |
    - alt             | false       | { String }          | The alt text for the image.               |
    - title           | false       | { String }          | The title attribute for the image.        |
    - aspect_ratio    | false       | { String / Number } | The aspect ratio of the image.            | 'x/y' or 'x.y'
    - max_width       | false       | { String / Number } | The maximum width of the image.           | '1000' or 1000
    - lazy            | false       | { Boolean }         | Whether the image should be lazy loaded.  | true or false
    - fetchpriority   | false       | { String }          | The fetch priority for the image.         | 'low' or 'high'
    - sizes           | false       | { String }          | The sizes attribute for the image.        | One or more strings separated by commas, indicating a set of source sizes ordered from large to small. Each source size consists of:
                                                                                                          1. A media condition. This must be omitted for the last item in the list.
                                                                                                          2. A source size value.
    - widths          | false       | { String }          | The widths attribute for the image.       | 'xxx, xxx, xxxx ...'
    - figcaption      | false       | { String }          | The caption for the image.                |
    - fallback_image  | false       | { String }          | The fallback image for the image.         | https://shopify.dev/docs/api/liquid/filters/placeholder_svg_tag

  # Recommendations:
    - The max_width parameter is recommended to ensure the srcset is not larger than the image.
    - The sizes parameter is recommended to ensure the image is displayed responsively.
    - Only use fetchpriority 'high' for images that are critical to the user experience (Above the fold).

  # Example:
    {%- render 'format-image',
      source: section.settings.image,
      lazy: true,
      sizes: '(min-width: 1200px) 50vw, 100vw'
    -%}
{%- endcomment -%}

It could also be something like this.

{%- comment -%}
  Files must meet the following requirements:
    https://help.shopify.com/en/manual/shopify-admin/productivity-tools/file-uploads

  # Accepts:
    - source
      required:       true
      type:           Object
      description:    The image to be displayed.

    - aspect_ratio
      required:       false
      type:           String / Number
      description:    The aspect ratio of the image.
      acceted values: 'x/y' or 'x.y'

  # Recommendations:
    - The max_width parameter is recommended to ensure the srcset is not larger than the image.
    - The sizes parameter is recommended to ensure the image is displayed responsively.
    - Only use fetchpriority 'high' for images that are critical to the user experience (Above the fold).

  # Usage:
    {%- render 'format-image',
      source: section.settings.image,
      lazy: true,
      sizes: '(min-width: 1200px) 50vw, 100vw'
    -%}
{%- endcomment -%}

I still think this could bloat the snippet quite a bit... Imagine something like this where the prop variable also works as an assign.

{%- liquid
  prop source = object | required | description: 'The image to be displayed.'
  prop aspect_ratio = string or number | description: 'The aspect ratio of the image.' | accepted_values: 'x/y' or 'x.y'
-%}
  
{%- prop source = object
  | required
  | description: 'The image to be displayed.'
-%}

{%- prop aspect_ratio = string or number
  | description: 'The aspect ratio of the image.'
  | accepted_values: 'x/y' or 'x.y'
  | default: 1.4
-%}

{{ aspect_ratio }} -> 1.4

Both have their up and downsides, but the possibility to make values required etc, can give nice theme checks as well.

@charlespwd charlespwd changed the title {%- render -%} autocomplete Support {% render 'snippet', %} named argument completion Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:theme-check enhancement New feature or request open for contributions theme bugs that won't be prioritized in the near future
Projects
None yet
Development

No branches or pull requests

2 participants