-
Notifications
You must be signed in to change notification settings - Fork 103
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
Describe criteria for link and form morphing #178
Open
seanpdoyle
wants to merge
1
commit into
hotwired:main
Choose a base branch
from
seanpdoyle:morph-visits
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgemanrubia is this guidance correct? Are the
[data-turbo-action]
attributes necessary, or should the resulting Visit get classified as a "page refresh" through a different mechanism? This guidance is based on boolean criteria in PageView.isPageRefresh.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanpdoyle same-location redirects are already
replace
by default as per this:https://github.com/hotwired/turbo/blob/c471974dfaf546c071924935ec74c6c419dbd6ec/src/core/drive/navigator.js#L161-L164
So the attribute is not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgemanrubia this documentation was mostly in response to confusion stemming from
<a>
- and<form>
-driven Page Refreshes.I've verified that redirects in the way you've described *do* work as expected:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further inspection, I realized the difference between #178 (comment) and #178 (comment).
The Navigator.getDefaultAction you've shared:
differs from the Visit.isPageRefresh comparison:
The
Navigator.getDefaultAction
compares URL.href values, whereas theVisit.isPageRefresh
compares URL.pathname.Put another way, redirects back require exact matches of the full URLs (including query parameters), while refreshing when the action is forced only compares the URL paths (excluding query parameters).
This script passes with the `data: {turbo_action: "replace"}` option, then fails without the option:
Does that inconsistency need to be resolved? Should replace actions become more flexible by comparing
URL.pathname
instead ofURL.href
? Should page refreshes become more strict by comparingURL.href
instead ofURL.pathname
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanpdoyle The motivation behind using
pathname
is here, although this also generates slight inconsistencies in mobile adapters that still only checkhref
. What is your opinion on this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think behavior should be consistent. I'm not sure I have enough practical experience to judge which side should change to match the other.
If it's determined that inconsistency is the best option, then I think a documentation change (like the one proposed in this PR) will be necessary to clarify what is and is not going to result in a Page Refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgemanrubia should determining Visit Action based on the URL (as described in #178 (comment)) be consistent in both
Navigator.getDefaultAction
andVisit.isPageRefresh
?If it should, do you have an opinion on which implementation should change to be consistent with the other?