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

Enable frame morphing for data-turbo-frame links and forms if the URL doesn't change #1316

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

botandrose
Copy link
Contributor

@botandrose botandrose commented Sep 22, 2024

Hi folks, this PR is aimed at enabling another specific use-case for morphing turbo-frame[refresh=morph] that isn't currently supported.

Status quo:

Right now, if one clicks a link or submits a form that contains data-turbo-frame, that target frame is replaced without morphing, even if refresh=morph. This makes sense if the frame is navigating to a different URL, because its not a "refresh" operation at all; it's more of a navigation operation.

Motivating use-case:

However, I propose that in some common idiomatic situations these operations are indeed refreshes, and therefore should be morphed. Imagine a turbo-frame with a list of blog posts, and an external new post form, which targets the blog post list. I submit for your consideration that in this situation, submitting the form is indeed refreshing the frame.

<turbo-frame id="posts" src="/posts" refresh="morph">
  <ul>
    <li><a href="/posts/3">Post 3</a></li>
    <li><a href="/posts/2">Post 2</a></li>
    <li><a href="/posts/1">Post 1</a></li>
  </ul>
</turbo-frame>

<form action="/posts" data-turbo-frame="posts">
  <input name="title">
  <input type="submit">
</form>

Proposed solution

If a morphing frame's URL changes from the result of a link click or form submission, keep the current behavior and render it with the normal replacement rendering. If the URL does NOT change, however, consider it a refresh and use morphing.

Final notes

Please let me know if this PR is an idea worth considering for merging, and if so I'll do a bit more work here. I think it could use some more tests to fully nail down all the permutations it implies.

Thanks for your time, and continued work on Turbo!

@@ -234,6 +233,9 @@ export class FrameController {
const frame = this.#findFrameElement(formSubmission.formElement, formSubmission.submitter)

frame.delegate.proposeVisitIfNavigatedWithAction(frame, getVisitAction(formSubmission.submitter, formSubmission.formElement, frame))
if (frame.src === response.response.url && frame.shouldReloadWithMorph) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Determining whether a page-wide Visit is to be handled as a refresh involves more than an exact comparison of URL values.

At the Page level, it involves comparing the URL.pathname values (ignoring differences in query parameters) as well as checking for whether or not the Visit.action is "replace".

This change could make a similar comparison at the [src] and URL level, but driving Frame elements does not have a concept of a Visit, and [data-turbo-action="replace"] has Page-wide navigation implications.

Does it make sense to compare pathnames and ignore query parameters?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disentangling [src] attribute changes from a "Frame Visit" initiated by a <form> element resulting in a redirect (instead of an <a> element click) isn't possible at the moment.

I had opened #430 to introduce the concept of a Frame Visit. It might be worth revisiting that initiative in hopes that the concept could make these sorts of implementation changes more straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanpdoyle Thanks for taking a look at this, and for pointing out how page refreshes already handle this situation regarding query params. I definitely want to try to keep things consistent between the frame and page systems! Not to mention that it will make this feature just that little bit more useful. I'll push a commit for this shortly.

Regarding #430, I love it. Initially wrapping my head around Turbo's internals has not been easy, and that additional abstraction would help to compartmentalize and clarify some of the complexity. I hope it finally gets merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants