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

Support form submissions in the ViewTransitions router #8963

Merged
merged 12 commits into from
Nov 8, 2023
Merged

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Oct 31, 2023

Changes

  • Allows the user to enable form submissions to be handled. Currently this is via a form option on the ViewTransitions router: <ViewTransitions form />. Probably there is a better name.
    • Presumably this would become the default in 4.0.
    • Also allows data-astro-reload like with anchors, to opt-out on a per form basis.
  • Adds an option to the navigate() API options.init which is a RequestInit. This is needed for forms to provide the body and set the method on the request.
    • If this is undesirable as a public API we could maybe make it a private option.

Testing

  • Tests added in the normal way
  • Added a test for when errors are thrown.

Docs

Copy link

changeset-bot bot commented Oct 31, 2023

🦋 Changeset detected

Latest commit: a2b67da

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 31, 2023
packages/astro/components/ViewTransitions.astro Outdated Show resolved Hide resolved
packages/astro/components/ViewTransitions.astro Outdated Show resolved Hide resolved
@@ -3,13 +3,18 @@ type Fallback = 'none' | 'animate' | 'swap';

export interface Props {
fallback?: Fallback;
form?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of using these props to select deviant behavior.
Looking at the uses, handleForms seems more intuitive?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe animateForms or something similar? Astro doesn't handle anything other than catching the submit and passing everything to the endpoint. "Handle" could be misunderstood like validation, error management, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use the word animate as animations are a use-case but not what the feature actually does. You can actually use this router and have no animations at all.

packages/astro/src/transitions/router.ts Show resolved Hide resolved
packages/astro/src/transitions/router.ts Show resolved Hide resolved
@martrapp
Copy link
Member

In the Navigate API, the Navigate event has a property for form data, but it is not clear from the specification how this property is set. I don't see a problem if Astro's navigate() has more options than the Navigate API's navigate(). But I would question the name and type of the option, see comments in code.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Can we add a test case with a failure when doing a submit? I would like to see/understand what's the expected behaviour in that case

@matthewp
Copy link
Contributor Author

matthewp commented Nov 2, 2023

@ematipico You mean if the API responds with a 400/500? I can add that.

@ematipico
Copy link
Member

@ematipico You mean if the API responds with a 400/500? I can add that.

Yeah exactly, just to get a sense of the expected behaviour. Cheers!

@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Nov 6, 2023
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Nov 6, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@matthewp
Copy link
Contributor Author

matthewp commented Nov 6, 2023

@martrapp I have updated the API per your suggestion to use formData as the navigation option.

@ematipico I've added a test which causes regular transitions for non-200 responses.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Looks great @matthewp ! Made some suggestions to the changeset below! Basically, I'm gonna need a bit more hypeeeee 🥳

.changeset/many-weeks-sort.md Outdated Show resolved Hide resolved

Form support in ViewTransitions router

The `<ViewTransitions />` router can now handle form submissions, allowing you to retain stateful UI. This feature is opt-in for semver reasons and can be enabled by adding the `handleForms` prop to the component like so:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `<ViewTransitions />` router can now handle form submissions, allowing you to retain stateful UI. This feature is opt-in for semver reasons and can be enabled by adding the `handleForms` prop to the component like so:
The `<ViewTransitions />` router can now handle form submissions, allowing you to trigger in-page transitions from a form submission while keeping page state. This feature is opt-in and can be enabled by adding the `handleForms` prop to the routing component:

I'm gonna need you to sell forms to me harder than this, Matthew. 😅

Can you do better than "retain stateful UI"? Something that more walks through the (exciting!) process itself? People are always asking about forms, can we hype this in a more "give the people what they want" kind of way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right, I needed the pep-talk. Pushed with a bit more, trying to emphasize that this addition completes the circle of everything you need to stay in the client if that's what you want.

.changeset/many-weeks-sort.md Outdated Show resolved Hide resolved
.changeset/many-weeks-sort.md Show resolved Hide resolved
.changeset/many-weeks-sort.md Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <[email protected]>
@martrapp
Copy link
Member

martrapp commented Nov 7, 2023

I'm happy with the fact that we're adding properties that come later from the Navigation API as additional fields in NavigationNavigateOptions. And the new property in ViewTransitions is now also more descriptive. Everything looks good to me!

The only thing I was wondering is if the popstate handler needs to be customized as well. After all, it always passes an empty options object. But I think it's fine the way it is. An app would prevent users from returning to the form after the "post". And with a multi-page form, you would disable the "forward" after navigating back.

@matthewp matthewp merged commit fda3a02 into main Nov 8, 2023
13 checks passed
@matthewp matthewp deleted the vt-form branch November 8, 2023 17:45
@astrobot-houston astrobot-houston mentioned this pull request Nov 8, 2023
natemoo-re pushed a commit that referenced this pull request Nov 22, 2023
* Support form submissions in the ViewTransitions router

* Align with navigate API, add `formData` option

* Change API to handleForms

* Add a changeset

* Add a test for non-200 responses

* Update .changeset/many-weeks-sort.md

Co-authored-by: Sarah Rainsberger <[email protected]>

* Update .changeset/many-weeks-sort.md

Co-authored-by: Sarah Rainsberger <[email protected]>

* Add a little more on why this is exciting!

* Update .changeset/many-weeks-sort.md

Co-authored-by: Sarah Rainsberger <[email protected]>

* Switch to e.g.

---------

Co-authored-by: Sarah Rainsberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants