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

fix: APP-410 save buy flow on last user interaction #2574

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

r41ph
Copy link
Contributor

@r41ph r41ph commented Jan 8, 2025

Description

https://regennetwork.atlassian.net/browse/APP-410

Now when users interact with the forms the information should be stored in the localstorage automatically even when the next button hasn't been clicked yet, and therefore, on navigating away and back to the same page, users should be in the same step and see the same form data as before navigating away.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. https://deploy-preview-2574--regen-marketplace.netlify.app/project/mai-ndombe-4/buy
  2. Edit the form and navigate away then come back to the same page and verify the step and the form data is the same as before navigating away.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Jan 8, 2025

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 0b294da
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/677e7b344833bc00097a419a
😎 Deploy Preview https://deploy-preview-2574--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 8, 2025

Deploy Preview for terrasos ready!

Name Link
🔨 Latest commit 0b294da
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/677e7b34c5ca2d0008031c43
😎 Deploy Preview https://deploy-preview-2574--terrasos.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@r41ph r41ph requested a review from blushi January 8, 2025 12:31
@r41ph
Copy link
Contributor Author

r41ph commented Jan 8, 2025

@erikalogie see testing instructions

@erikalogie
Copy link
Collaborator

LGTM

@r41ph r41ph force-pushed the fix-APP-410-save-buy-flow-on-last-user-interaction branch from fef3652 to 0b294da Compare January 8, 2025 13:18
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Two additional comments:

  • We don't save values for the last step to local storage
  • When using credit card, if the user leaves the page while being on the last step, when he comes back, we should go back to step 2 (Payment info) because we need the user to re-enter credit card info since this is something we can't save to local storage of course

...data,
creditsAmount: _creditsAvailable,
currencyAmount: _spendingCap,
sellOrders: orderedSellOrders,
Copy link
Member

Choose a reason for hiding this comment

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

why aren't we setting this to the same value as in setValue(SELL_ORDERS, ...) just above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good you noticed this, thanks

{
...data,
creditsAmount: currentCreditsAmount,
currencyAmount: currencyAmount,
Copy link
Member

Choose a reason for hiding this comment

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

why not updating sellOrders too?

{
...data,
creditsAmount: currentCreditsAmount,
currencyAmount: isNaN(value) ? 0 : value,
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Comment on lines +102 to +110
updateMultiStepData(
{
...data,
currency,
paymentOption,
currencyAmount: 0,
},
activeStep,
);
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?
we update the currencyAmount but not the sell orders which depend on it, which could lead to inconsistent data in local storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm updating the multistep data here to keep it in sync with the input field, don't you think this needs to be done?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure since it seems like this will be updated anyway from handleCurrencyAmountChange with the correct values for corresponding sell orders and credits input

Comment on lines +102 to +103
name: form.watch('name'),
email: form.watch('email'),
Copy link
Member

Choose a reason for hiding this comment

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

I believe form.getValues https://react-hook-form.com/docs/useform/getvalues would be more appropriate in this case, since we don't need to subscribe to value changes in this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants