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

Update the history before rendering promoted frames #618

Merged
merged 23 commits into from
Jul 19, 2022
Merged

Update the history before rendering promoted frames #618

merged 23 commits into from
Jul 19, 2022

Conversation

manuelpuyol
Copy link
Contributor

@manuelpuyol manuelpuyol commented Jun 30, 2022

Closes #617

I'm extracting the history method selection to getHistoryMethodForAction in `util and using it in both Frames and Visits.

In case the frame needs to change the history, it update it before rendering the frame's content, and will call the proposed visit with updateHistory = false, which will avoid a second history update.

<div>
<a id="drive_enabled" href="/src/tests/fixtures/tabs.html">Tab 1</a>
<a id="drive_enabled" href="/src/tests/fixtures/tabs/two.html">Tab 2</a>
<a id="drive_enabled" href="/src/tests/fixtures/tabs/three.html">Tab 3</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to declare these anchors with an [id] attribute? If it is, could we make sure their values don't collide with other elements on the page?

Also, are there plans to add tests to cover the new behavior and to guard against regressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I wanted to validate the idea and implementation and will follow up with some tests

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 I added some tests in e99e69e...16b3bdd, but it's really hard to know the state of tests already in place since there's a lot of flakiness. I'm different seeing test pass and fail each run

Copy link
Contributor

@seanpdoyle seanpdoyle left a comment

Choose a reason for hiding this comment

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

Thank you for opening this issue and following it up with a PR!

@manuelpuyol
Copy link
Contributor Author

@seanpdoyle anything you'd like me to do to move this forward?

@manuelpuyol
Copy link
Contributor Author

@seanpdoyle I added event types and fixed the IDs

src/globals.d.ts Outdated
@@ -11,3 +16,58 @@ interface Window {
Turbo: typeof import("./core/index")
SubmitEvent: typeof Event
}

interface CustomEventMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is covering a lot of the same territory as #452.

I'm in favor of introducing these types, but this batch of work seems tangential to Frame promotion.

I'm sorry if my casting suggestion sent you down this road, that was not my intention!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think is the best path forward? I can leave only the BeforeFrameRender as a type here, or I could do something similar to #452, exporting the type in session.ts and typing the dispatch call.
or we can roll this back and continue with the test casting the event manually

@manuelpuyol
Copy link
Contributor Author

@seanpdoyle I'm rolling back the type changes in favor of #452. I'm bringing the as CustomEvent back while that's not merged. I don't think the casting is harmful especially because it's only in a test

@dhh
Copy link
Member

dhh commented Jul 15, 2022

Can you rebase/update the tests?

@dhh dhh added this to the 7.2.0 milestone Jul 16, 2022
@dhh
Copy link
Member

dhh commented Jul 18, 2022

Can you remove the yarn.lock changes?

@manuelpuyol
Copy link
Contributor Author

done!

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.

Turbo should update the URL before frames render
3 participants