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

LF-4022: [spike] investigate current and desired state for navigation #3158

Conversation

antsgar
Copy link
Collaborator

@antsgar antsgar commented Mar 13, 2024

Description

Examples of proposal of changes for navigation handling throughout webapp.

Example 1: Multistep forms with use of useHookFormPersist

There are several instances of multistep form where:

  • Each step has its own URL.
    -useHookFormPersist and HookFormPersistProvider are used to handle back and forth navigation throughout the steps and to preserve the data when navigating.
  • Both browser navigation and in-app navigation between steps are allowed, but with some rules such as disallowing forward navigation once the form has been cancelled. More details here

This brings a set of problems:

  • Complex logic within the useHookFormPersist hook that is hard to debug when issues with navigation occur.
  • Need to send out paths programatically to the Redux store on each navigation instance to make sure it gets recorded to the navigation stack.
  • Need to persist form data to the Redux store so that it's preserved with back and forth navigation.
  • Lots of code repetition for behavior and some components of the UI which are common between forms.

The example I used to illustrate my proposal for this is the add expense form, which can be accessed from the Transactions screen by clicking on the "Add" button and choosing "Expense".

Proposal:

  • We should either go for a single route multistep form, where there are no separate URLs for steps and we rely solely on in-app navigation (this is what is shown in this branch), or alternatively, keep the multiple URLs but allow browser navigation to happen normally both back and forth, including forward navigation once the form has been cancelled (this option was explored up until commit 4c10a8d2401225d3ba473e92348363d4df9f94e7 in this branch). Fiddling with the history in a complex way to change the default behavior of browser navigation seems like a bad idea both from the user experience and the technical perspective.
  • Navigation and data persistence in this proposal is handled through a React component, MultiStepForm. This component handles displaying the progress bar, title of each step and back and cancel buttons to avoid repetition, and renders the contents for each of the steps in the form. It also calls the useForm hook from react-hook-form and preserves the data entered by the user in the form. Each form does not need to call useForm but gets passed the form from this parent form component, and can read persisted data or get needed methods from it. Note that there isn't a need to store the persisted data to the Redux store in order for it to be preserved between steps -- react-hook-form already handles this with the shouldUnregister prop. When shouldUnregister is set to false (which is the default), the data is preserved even if the inputs are unmounted. Use of useHookFormPersists and HookFormPersistProvider is altogether removed in this example.
  • When we need to create a new multistep form, we can create a form that calls this new component and sets the step titles and contents. Within the contents of each step we won't need to handle navigation, and we'll have access to persisted data. We'll also have an onGoForward function available to be able to navigate to the next step from each step contents (this couldn't be abstracted since UI for the navigation from one step to the next differs from one form to the next). We'll only need to worry about calling this function to move the user forward, read the data from the form and handle any interdependecies between steps (in the case of the add expense form, the second step displays different inputs depending on the expense types seleted on the first step) and submit the data on the last step.
  • The new component also includes a ClickAwayListener so that the cancel modal will be prompted when the user tries to navigate away, even if not by clicking directly on the Cancel button.

Example 2: Multiple UIs for the same route, using navigation to update component state

This example has now been cherry-picked to a separate branch and can be viewed in PR #3173

In the CustomSignUp component, we're using a single route and a single component to display multiple different UIs situationally, but using manual navigations to update the state so that we "emulate" multiple routes and allow the user to go from one step to the previous one when using browser navigation. This is another instance where we have logic that's complex to debug and error prone with the purpose of altering the default browser navigation behavior. Essentially here I think we need to make a decision, just like in the first example -- we either go for a single route showing different steps, or we go for separate URLs for each step, but in both cases we should let the browser navigation behave normally, and in both cases we should avoid the current pattern which is using the navigation state as a replacement for actual state either in components or in Redux.
If we go for a single route, user won't be able to navigate back and forth between steps by using the browser, and if we use multiple routes then they'll be able to navigate back and forth freely. In this last case they'll also have feedback that a navigation has occurred and they'll know which step they're at by seeing the URL, as opposed to the current situation where they'll be navigating back and forth multiple times to the same URL which can be confusing. In both cases we should also avoid the current pattern which is using the navigation state as a replacement for actual state either in components or in Redux.
In this branch, I went for the first scenario which is a single route. Some highlights of the change are:

  • No navigation handling, all the steps are rendered in the same route and browser navigation is not possible.
  • Instead of using navigation state to store the user data and the step which should be displayed, that data is sent to the Redux store.
    Alternatively we could:
  • Setup one route for each component.
  • Handle navigation between steps, no need to store the component to show anywhere.
  • User data could still be stored in the Redux store.

Jira link:
https://lite-farm.atlassian.net/browse/LF-4022

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@antsgar antsgar mentioned this pull request Mar 28, 2024
16 tasks
@kathyavini kathyavini marked this pull request as ready for review April 2, 2024 15:58
@kathyavini kathyavini requested review from a team as code owners April 2, 2024 15:58
@kathyavini kathyavini requested review from Duncan-Brain, kathyavini and SayakaOno and removed request for a team April 2, 2024 15:58
Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

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

It looks like we need to update Add revenue as well?
Screenshot 2024-04-02 at 1 31 32 PM
Screenshot 2024-04-02 at 1 32 29 PM

kathyavini and others added 3 commits April 3, 2024 09:32
Co-authored-by: Sayaka Ono <sayaka.ono.81@gmail.com>
The open state and setter are passed from the new form wrapping component now, but the old forms aren't wrapped by that component yet
Restores PureFinanceTypeSelection's compatibility with the old form flow
@SayakaOno SayakaOno mentioned this pull request Apr 22, 2024
16 tasks
@github-merge-queue github-merge-queue bot closed this pull request by merging all changes into integration in 94dceed Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants