-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix ignored boolean attributes getting set by the server #4050
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
Conversation
|
I agree that the current behavior is confusing. I'm only worried that some people might already rely on this behavior, so maybe we need to make it opt in, like |
|
fwiw I was thinking about a |
|
Hmm. That's a good point about tracking client side changes. I could probably work around the issues I'm seeing by using js commands to alter attributes instead of using direct dom manipulation. I still don't think ignore attributes works in an expected manner. How do you feel about adding this check only for non-wildcard ignores? Imo the fact that removing |
|
It's tricky. I think if we change it, we should change it for all attributes and not handle wildcards differently. I'll need to think about it for a bit. |
Adds `phx-update="track-attributes` and `phx-track-attributes` attribute where the first one tracks attributes on the whole subtree and the second one only for the element that set the attribute. Using this makes LiveView keep clientside changes until a server overrides an attribute value with a new one. This is useful when clientside libraries rely on attributes they set, but `phx-update="ignore"` is too much. This is a proof of concept, I'm not sure we actually want to go with this. For simplicitly, I vendored morphdom into the repo. The important change there is a new `proxyFromEl` callback + changes to morphAttrs. Relates to: #4049 Relates to: #4050 (comment)
|
Let's go ahead with this, please just adjust the failing test :) |
|
🙌🏻 |
This breaks tests around wildcard behaviour, which however I'm not sure I agree them testing behaviour I'd expect.
Closes #4049