-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Omit meaningless markup from hidden input in core components #5700
Omit meaningless markup from hidden input in core components #5700
Conversation
name={@name} | ||
id={@id} | ||
value={Phoenix.HTML.Form.normalize_value(@type, @value)} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want a {@rest}
in here to capture any additional provided attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest consists of things that should not be in hidden input as well. At least to my understanding. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/hidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing something like form
can still be relevant for hidden inputs, therefore rest should be included in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
For what is worth, I don’t think we should support :hidden through |
In other words, I would remove hidden from here: https://github.com/phoenixframework/phoenix/blob/main/priv/templates/phx.gen.live/core_components.ex#L277 |
We should remove radio as well to solve #5506 . |
I tried it too, leading to an unsafe input error. I am AFK to reproduce that error exactly, but it is something like that. |
Yep, the exact error is
|
@bopm you will have to do something like this when using
You cannot pass the form field to a plain input tag, that's where the error comes from. |
@SteffenDE That works, but do I understand correctly that this way I am losing things like |
That's right, but normalize_value does nothing for "hidden" inputs anyway. As this PR is only about what is generated in new applications, you can of course keep the "hidden" case for the |
That makes sense too, my other concern is that this general approach that @josevalim is offering will push developers to remember that some field types require different notation than others. For me, |
I squeezed it all into a one liner, so maybe it is more amenable for merging. |
Motivation
The current version of core components given
produces next markup
Most of that is completely meaningless for hidden fields.
Change
After implementing these changes, it'll generate next markup: