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

Page refreshes: Use pathname instead of href to detect if locations are equal #1079

Merged

Conversation

brunoprietog
Copy link
Contributor

Before this commit, URLs had to be exactly the same for the possibility of a refresh with morphing.

Now, it only checks that the paths are equal, regardless of whether they have different search params.

From my perspective, search params don't determine whether it is a refresh or not. The most representative example that comes to mind is the same case as a Rails controller. A show action could have different search params, but in essence, it remains the same action and the same page.

Additionally, sometimes when clicking a link on social media, for example, tracking parameters are injected. If these parameters exist and a form is submitted, Turbo would not detect that it is a refresh.

These are just small examples of use cases, but obviously there are many more.

…re equal

Before this commit, URLs had to be exactly the same for the possibility of a refresh with morphing.
Now, it only checks that the paths are equal, regardless of whether they have different search params.
Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

I think this makes a lot of sense @brunoprietog 👏 . I left a minor suggestion there. I'll merge it with that. Thanks!

src/core/drive/page_view.js Show resolved Hide resolved
Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Thanks @brunoprietog 👏

@jorgemanrubia jorgemanrubia merged commit 06ee1fe into hotwired:main Dec 7, 2023
@brunoprietog brunoprietog deleted the use-pathname-for-refreshes branch December 7, 2023 07:02
@krschacht
Copy link

@jorgemanrubia or @brunoprietog In my application I have multiple cases where I want the Page Refresh morph action to be used, but the routes differ in a more significant way than simply search params. But currently, the page will only morph when it redirects back to the same route.

I think that I have good reason for my routes to be different, but then it falls back to Turbo Drive's full body replacement. I find myself contorting my code in order to redirect back to the same route just so I can get the Page Refresh morph to work.

Should we also add a way for morph to be explicitly triggered by including data-turbo-action="morph" in a form or in a link_to? I think the automatic default is very reasonable, but it doesn't feel right that the automatic default is the only way.

(Maybe it's not right to have this discussion on this pull request but this seems to be the most recent place that the question of route matching / page refresh was being considered.)

@brunoprietog
Copy link
Contributor Author

Yes, I've also seen the need to do so. My most representative case is a page with several tabs, in which to maintain order, I have each tab in its own controller. I had to unify them in one and differentiate them using the tab parameter, but it's certainly not the most comfortable.

The argument for not doing it is that since morphing is an expensive operation, it shouldn't need to be done when the current page doesn't look like the new page. But as we have seen, this is not always the case. Just because the path isn't the same, doesn't mean that the pages don't look similar enough to want to morph. Over time, I feel this will incentivize the use of fat controllers with parameters that differentiate the different use cases, just to be able to take advantage of morphing.

I also understand that it's a cost to introduce a new element to the Turbo API, and that is also a valid point.

I would also love to be able to make Turbo frames navigation with morphing for accessibility reasons. I'm currently doing it but using Idiomorph separately.

@krschacht
Copy link

@brunoprietog Thanks for the quick reply. I'm glad I'm not crazy. :) I too have started to create fat controllers with parameters that differentiate the use-cases in order to solve for this.

@jorgemanrubia I tee'd up a PR that adds this functionality. It's my first time contributing to rails/hotwired/stimulus so I hope I followed the guidelines correctly: #1145 . I welcome any guidance on how I implemented it.

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.

None yet

3 participants