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

[5.x] Refactor Form and SVG tags to use :attr prefix instead of $knownTagParams #9576

Merged
merged 5 commits into from
Feb 26, 2024
Merged

[5.x] Refactor Form and SVG tags to use :attr prefix instead of $knownTagParams #9576

merged 5 commits into from
Feb 26, 2024

Conversation

JohnathonKoster
Copy link
Contributor

This PR closes #9430

@jasonvarga jasonvarga merged commit 5b07862 into statamic:master Feb 26, 2024
18 checks passed
@jasonvarga jasonvarga deleted the remove-known-tag-params branch February 26, 2024 18:41
@robdekort
Copy link
Contributor

robdekort commented Apr 10, 2024

I just ran into this. A couple of notes.

  1. I read the text in the upgrade guide but the title ### Tag parameters that output HTML attributes are opt-in didn't trigger anything in my brain. Maybe it helps to note in the title this regards the form and svg tags.
  2. This is a really tedious change I suspect people are going to miss and it will break all their SVG's and possibly forms. Perhaps this could be automated in an update script. Where all none svg/form tag parameters get prefixed with attr:. It might also notify users about this automated change during the upgrade so folks are aware and that it could've missed instances.

@sjclark
Copy link
Contributor

sjclark commented Apr 10, 2024

Feels a little fiddly - out of curiosity whats the logic behind this? Is it a security thing? And just confirm will this affect all tags that use class parameters or just svg & forms?
ie. {{svg attr:class="whatever"}}
But {{glide class="whatever"}}?

@jasonvarga
Copy link
Member

I read the text in the upgrade guide but the title ### Tag parameters that output HTML attributes are opt-in didn't trigger anything in my brain. Maybe it helps to note in the title this regards the form and svg tags.

The upgrade guide explains exactly who/what this affects under each heading. We hope people will read those too and not just the headings.

CleanShot 2024-04-10 at 09 45 48

will this affect all tags that use class parameters or just svg & forms?

Any tags that output html attributes.

  • svg
  • form:create
  • user:login_form
  • user:register_form
  • etc

But {{glide class="whatever"}}?

The glide tag never supported classes. There's no change there. It does support tag="true" which output an tag, but you could never slap classes on it. Only alt.

whats the logic behind this?

It's to avoid ambiguity between tag parameters that control behavior and params that just output html attributes.

For example, if the glide tag did support html attributes (it could easily be added), how would we tell the difference between you wanting to control the width of the resized image vs. the width attribute on the img tag?

{{ glide:img tag="true" width="100" }}
// img.jpg?w=100 or <img width="100"> ?

The place where we actually ran into this was in this PR where we add a @-syntax to prevent parsing Antlers in parameters (we use the @ already to prevent parsing elsewhere so this matched that style).

{{ tag @x-data="{ open: false }" }}

Having the @ symbol mean preventing parsing would stop you from doing stuff like Vue/Alpine event handlers like @submit="". So, we made it obvious by adding the prefix attr:@submit="".

This is a really tedious change

It sounds tedious, but have you actually attempted it yet? In Peak, there are only 4 {{ svg }} tags.

@robdekort
Copy link
Contributor

robdekort commented Apr 10, 2024

I understand the change. And it makes sense. And a major version bump is the right time for this of course. However...

It sounds tedious, but have you actually attempted it yet? In Peak, there are only 4 {{ svg }} tags.

Well, it's a starter kit so pretty lean in that regard. In my most recent projects I have up to 30 SVG tags. Considering I probably need to update 20+ sites, I can guarantee you it'll be tedious. If it is what it is, it is what it is.

The upgrade guide explains exactly who/what this affects under each heading. We hope people will read those too and not just the headings.

Sure, me too, but this is just feedback hoping to improve the language. Tag parameters that output HTML attributes are opt-in is a quite technical way of saying this. My suggestion is to make this a bit clearer. Because I read it and thought it didn't apply to me.

Perhaps the changes could be listed out per tag that it applies to. That way you get the tag name in the heading. I'm thinking this could avoid frustration.

@sjclark
Copy link
Contributor

sjclark commented Apr 10, 2024

@jasonvarga thanks for explaining - for some reason I thought glide batch had it but have completely hallucinated it 😂 understand the logic around it changing (and thanks for looking into it in the other threads referenced etc)

@jasonvarga
Copy link
Member

FYI this has been completely reverted in 5.0.0.alpha-2. You don't need to change anything.

@robdekort
Copy link
Contributor

Noticed, cool 😃. I'll make it a good practice to do it from now one however.

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.

Use attr: prefixes instead of $knownTagParams in all form tags
4 participants