Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[PWA-411] Updating quantity value when initialValue changes. #2353

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

revanth0212
Copy link
Contributor

Description

The quantity stepper field in ProductListing retrieves its initial state from Apollo, but when that data is updated via other mutations on the page, the new value is not being synced with form state. This is because the initialValue is not used to update the internal value when initialValue changes.

Ideally, I wouldn't have called it initialValue, because truly initialValue should only be used while initializing the component but in this case, it changes every time. In this case, it actually is the quantity value on the product. Maybe we should consider calling it form value or something appropriate instead.

Let me know what you guys think.

Related Issue

Closes PWA-411

Verification Stakeholders

@jimbo
@tjwiebell

Verification Steps

  1. Add Configurable Product to cart and go to Cart Page.
  2. Edit that item and adjust the quantity.
  3. Quantity should be updated on the cart page as well.

Checklist

None

@revanth0212 revanth0212 added the version: Patch This changeset includes backwards compatible bug fixes. label Apr 22, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Apr 22, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-411.

Generated by 🚫 dangerJS against 1adb326

@supernova-at
Copy link
Contributor

Code looks great, thanks for adding that test 💯

And yeah I agree, it is inconsistent that this initialValue is being used for more than just the initialization of the Quantity component's value.

Honestly in looking at the code, I'm surprised that the input field's value isn't being tracked by useState (and set where appropriate):

const [value, setValue] = useState(initialValue);

and then in quantity.js, we would just use that value to display (as opposed to having to set it explicitly via useFieldApi):

const { maskInput, value } = talonProps;
const textInputFieldState = {
  maskedValue: maskInput(value)
};

// and then in JSX...
<TextInput
  // other stuff
  fieldState={textInputFieldState}
/>

That whole textInputFieldState is admittedly a bit quirky, but that's mostly a byproduct of using informed.

All that said, when I implemented the above the bug here still persisted 😝 . I didn't dig into it too deeply though, it might be just wiring up the Quantity component on the page to know when to update (I think when the product quantity in Apollo updates, the component should update ... not sure why it's not).

@revanth0212
Copy link
Contributor Author

Spot on Mr.Terranova. I don't want to scope creep this PR with the stuff that deals with something far bigger in scope. Shall we handle that in a diff PR? How does that sound?

@dpatil-magento dpatil-magento merged commit 4da01a1 into develop Apr 26, 2020
@dpatil-magento dpatil-magento deleted the revanth/quantity_update_fix branch April 26, 2020 16:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:peregrine version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants