Consolidate morphing functions into core/morphing
#1438
Conversation
Follow-up to #1319 Prior to this commit, the `core/morphing` module defined and exported the `morphElements` and `morphChildren`, but did not include the logic for `MorphingPageRenderer.render` and `MorphingFrameRenderer.render`. Instead, those static methods imported and invoked the necessary functions. For the sake of consolidating all morphing logic (`Idiomorph` options, event dispatching, etc.), this commit moves the logic to the `core/morphing` module, then imports those functions back into the `MorphingPageRenderer` class (`morphBodyElements`) and the `MorphingFrameRenderer` class (`morphTurboFrameElements`).
There was a problem hiding this comment.
I really like the idea, but now I'm wondering if we can reduce it to simply Turbo.morph and frame.morphWith, leaving all the morphing helpers private, as they were before. I feel that this makes the API a bit more contained. Am I missing something that I'm not considering? What do you think?
| * @param currentFrame FrameElement destination of morphing children changes | ||
| * @param newFrame FrameElement source of morphing children changes | ||
| */ | ||
| export function morphTurboFrameElements(currentElement, newElement) { |
There was a problem hiding this comment.
What do you think about adding turboFrame.morphWith(newElement) instead? It feels more similar to element.replaceWith(newElement).
| * @param currentBody HTMLBodyElement destination of morphing changes | ||
| * @param newBody HTMLBodyElement source of morphing changes | ||
| */ | ||
| export function morphBodyElements(currentElement, newElement) { |
There was a problem hiding this comment.
I'm not 100% convinced about this name. You might want to use this method in something like turbo:before-render to replace something like div#app instead of body. But it's true that this event also gives you newBody as a parameter. I feel like the difference between morphBodyElements and morphElements is very subtle. One is simply lower level than the other, but we expose them at the same level.
What do you think if we rename this to just morph, so it becomes Turbo.morph(currentElement, newElement, { action: "replace|update", refreshFrames: true|false }), and leave morphElements as private? It'd be something similar to Turbo.visit(url, { frame: "id" }). Also, that way, we'd always dispatch the turbo:morph event. Then the morph method of the Turbo stream actions could also use that method. Perhaps the names of the option params could be improved, but they come pretty close to the idea of Turbo stream actions.
2 participants
Follow-up to #1319
Prior to this commit, the
core/morphingmodule defined and exported themorphElementsandmorphChildren, but did not include the logic forMorphingPageRenderer.renderandMorphingFrameRenderer.render.Instead, those static methods imported and invoked the necessary functions.
For the sake of consolidating all morphing logic (
Idiomorphoptions, event dispatching, etc.), this commit moves the logic to thecore/morphingmodule, then imports those functions back into theMorphingPageRendererclass (morphBodyElements) and theMorphingFrameRendererclass (morphTurboFrameElements).