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

Docs: attrs tag default value #26

Merged
merged 4 commits into from
Mar 4, 2023
Merged

Docs: attrs tag default value #26

merged 4 commits into from
Mar 4, 2023

Conversation

jlopinto
Copy link
Contributor

@jlopinto jlopinto commented Dec 2, 2022

I found that we can provide a default value to attrs tag values using the var tag
I'm not sure if it was initially designed to work like that.

This PR is about documenting that find.

I came across this use case and it happens to work "out of the box".

Perhaps this deserves additional tests before being documented?

```twig
{# image component #}
{% var loading="lazy" %}
Copy link

@luanfonceca luanfonceca Dec 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jlopinto, I don't think it's a default value per se, what's happening is that you're "redefining" the loading with the value of "lazy".

If you try something different than "lazy" the loading will still be set to "lazy":

{# Usage #}
{% image src="..." alt="..." loading="eager" %}

{# Output #}
<img src="..." alt="..." loading="lazy" />

But would be really cool if we could pass default values when defining the {% attrs %}, something like:

<img {% attrs src loading="lazy" alt %}>

Which unfortunately doesn't work, it's kind of a short version of yours. A nice alternative for that is to use the |default template filter to achieve that:

Suggested change
{% var loading="lazy" %}
{% var loading=loading|default:"lazy" %}

@jlopinto
Copy link
Contributor Author

Thank you @luanfonceca

I have made the necessary updates to the PR following your feedback.

Copy link
Owner

@mixxorz mixxorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@mixxorz mixxorz enabled auto-merge (squash) March 4, 2023 06:55
@mixxorz mixxorz merged commit efccede into mixxorz:main Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants