Bugfix/enable turbo stream morph on text #1225
Conversation
|
CC: @jorgemanrubia for visibility |
There was a problem hiding this comment.
Thank you so much for fixing @hcdeng! A very minor comment below:
These bugs are one of the risks that @seanpdoyle pointed out for having two pieces of code implementing the same logic. This will be a nice refactor to do, although it's not as as simple and extracting and reusing the function: we need to rework the internals of how we are handling turbo frames too.
src/core/streams/actions/morph.js
Outdated
| @@ -26,7 +26,10 @@ function beforeNodeRemoved(node) { | |||
| } | |||
|
|
|||
| function beforeNodeMorphed(target, newElement) { | |||
| if (target instanceof HTMLElement && !target.hasAttribute("data-turbo-permanent")) { | |||
| if (!(target instanceof HTMLElement)) { | |||
There was a problem hiding this comment.
Could you please change it to wrap with the condition instead of a clause guard:
if (target instanceof HTMLElement) {
...
}I prefer to keep things consistent there, and, also, I think Turbo's codebase tends to avoid guard conditions (that's a just a matter of style preference).
There was a problem hiding this comment.
Updated in 9c2bd18, thanks!
I had the chance to test out @omarluq 's fantastic addition of the Turbo Stream morph action in #1185. The feature works as expected, except fails to morph text-only changes. This appears to be due to a misreading of the Turbo-Rails implementation of the
beforeNodeMorph/#shouldMorphElementcallback: https://github.com/hotwired/turbo-rails/blob/102a491754d46f7dd924201fcfaf879a0f04b11c/app/assets/javascripts/turbo.js#L3830-L3844.This PR updates the
beforeNodeMorphcallback to match the Turbo-Rails implementation -- with a tweak to hopefully increase readability -- and enable text-only morphing.