Update the history before rendering promoted frames#618
Update the history before rendering promoted frames#618dhh merged 23 commits intohotwired:mainfrom manuelpuyol:frame-history-update
Conversation
src/tests/fixtures/tabs.html
Outdated
| <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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yeah, I wanted to validate the idea and implementation and will follow up with some tests
There was a problem hiding this comment.
@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
seanpdoyle
left a comment
There was a problem hiding this comment.
Thank you for opening this issue and following it up with a PR!
|
@seanpdoyle anything you'd like me to do to move this forward? |
|
@seanpdoyle I added event types and fixed the IDs |
src/globals.d.ts
Outdated
| SubmitEvent: typeof Event | ||
| } | ||
|
|
||
| interface CustomEventMap { |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
|
@seanpdoyle I'm rolling back the type changes in favor of #452. I'm bringing the |
|
Can you rebase/update the tests? |
|
Can you remove the yarn.lock changes? |
|
done! |
Closes #617
I'm extracting the history method selection to
getHistoryMethodForActionin `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.