Display progress bar on form submissions

[terracatta]

Fixes #61

Note: This PR is simply an extraction from the unmerged PR #164.

Description

Currently when forms are submitted using Turbo Drive there is no progress bar shown when the request is taking a bit. This PR fixes that by bolting on two new formSubmission methods to the generic Adapter interface and implementing them in the browser adapter.

Android/iOS considerations

While I know extending the adapter interface could make extra work for the turbo-ios and turbo-android maintainers, I think there might be enough differentiated behavior in form submissions to make separate methods a better choice in the long-term vs genericizing the visitRequestStarted and visitRequestFinished methods.

[dhh]

cc @jayohms

[jayohms]

@dhh Is this the behavior that we want for all form submissions? I think there will be situations where it will visually make more sense to localize the progress indicator in/around the form, especially if the form is in a small portion of the screen.

For example, in HEY, we display a loading indicator directly on our submit buttons during the Sign Up flow (among other places). With the approach in the PR, is there an option available to easily customize the behavior?

In the HEY mobile apps, we currently use the turbo:submit-start and turbo:submit-end events to display a circular progress indicator in the top toolbar. I'm not opposed to extending the mobile adapters, however I doubt the native libraries would be able to provide a default form submission progress indicator. But we can make it easy to customize your own app to display progress.

[dhh]

I think having a progress bar as a default on the same heuristics as visits, like it doesn't kick in until 200ms or whatever it is, is good. But yes, you should be able to cancel that entirely if you have your own setup. Just that by default we should mimic what the browsers do naturally, just like we do with visits.

[philipithomas]

Just chiming in that it wasn't clear to me that the progress bar would not appear on form submissions, and I wish I could enable it (like this PR!).

Thinking about the first-time user experience, it seems to me that slow POST requests would happen often before slow GET requests.

[dhh]

@jayohms Does this need updating of the native adapters before we can merge?

[dhh]

@terracatta Can you bring the tests over as well?

[jayohms]

The mobile adapters will need to be updated to support the API, which isn't a big deal - however there will be adapters in the wild that will not have the updated adapter methods. So this PR will need to account for the fact that the adapter methods might not be available. We don't want javascript errors to break requests.

[dhh]

@jayohms, that's kinda what I thought. @terracatta could you have a look at making sure this only calls this method if the adapter has implemented it?

[terracatta]

@terracatta could you have a look at making sure this only calls this method if the adapter has implemented it?

@dhh yep, I'll take a look this weekend.

[terracatta]

@dhh ok, I added back the tests and added a check to make sure the adapter implements the methods before calling it.

As for your comment about being able to override this behavior if you have your own setup. What did you have in mind? Something like calling Turbo.disableProgressBarOnForms()?

[dhh]

This looks good. Would just like to validate that it indeed does not throw any halting JS errors with the native adapters.

@jayohms We don't actually need any specific opt-out, given that the adapter could just not implement it, right?

[jayohms]

We don't actually need any specific opt-out, given that the adapter could just not implement it, right?

Correct, we don't need a specific opt-out. The native libraries will end up implementing the js adapter methods and expose native APIs to the apps, but the UI behavior will be up to each app to customize on the native side.

Would just like to validate that it indeed does not throw any halting JS errors with the native adapters.

The approach is safe with the native adapters 👍

[terracatta]

@dhh and @jayohms My test suite isn't working great locally (it crashes during the FireFox tests for unrelated reasons) but I think we should see things passing if we kick-off the tests one more time.

Unless the tests continue to have issues, I think we are ready to rock!

[trsteel88]

@dhh, it doesn't look like this is in v7.1.0. Will there be a 7.2.0 tagged soon?