[PWA-178] Gift options support in cart page

[revanth0212]

Description

Gift Options Support in Venia Cart Page

Related Issue

Closes PWA-178

Verification Stakeholders

@soumya-ashok
@jimbo

Verification Steps

1. Add an item to the cart and go to the cart page.
2. Expand the See Gift Options section.
3. You should see the Gift Options.

Screenshots / Screen Captures

Checklist

• GQL does not support Gift Options yet. Make sure a rest call is made to save the options.

[PWAStudioBot]

	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-178.

Generated by 🚫 dangerJS against 89641b3

[sirugh]

I left a bunch of feedback, and we can discuss in person if you'd like.

I also noticed that the cache state is incorrect (and over-cached?) Look at the nested gift_options object which seems to be a copy of the parent but flips a flag?

[sirugh]

I'm not sure if these can be async but you may consider moving the REST calls here to the resolvers. Basically you would only interact with graphql in the component/talon but the resolvers would actually handle doing the rest call and update the cache accordingly.

[revanth0212]

Resolvers are temporary. I went with the easiest temporary option here. Making a network call from the hook gives me more control over when to make the call, for instance in case of gift message only send the request every 10 seconds. Yes, I can do this from the resolver as well, but the resolver can not differentiate between the checkbox and gift message.

[sirugh]

The flow I imagined was similar to how we used actions.

User input > call mutation (or invoke action) > call rest > augment cache (or redux store) with rest results

Right now we're doing both sort of simultaneously, which isn't necessarily wrong I guess, but it puts all the logic of doing the fetch within the component in a manner that we're trying to move away from in the long run.

[sirugh]

@tjwiebell @jimbo lemme know if I'm trying to scope creep here.

[sirugh]

This isn't relevant anymore since as of now we are only storing gift option choices and text to local storage through Apollo. When we work on Checkout we will need to make sure to add some step that pulls this data from the cache and submits it via REST when you click the checkout button.

[jimbo]

I would normally have done the API call on submit of the "step", which is Proceed to Checkout or some variation of Continue. With the way the forms are broken out, though, it may be easier to wait until Place Order.

[jimbo]

Code looks good to me. Only a couple of very minor things to change. 👍

Remember to look at other components in the codebase for guidance; some patterns not enforced by the linter are pretty consistent here nonetheless.

I'll test it out while you're fixing it up, then return to approve.

[jimbo]

Don't bother memoizing this.

[revanth0212]

Sure. Any particular reason?

[jimbo]

Just consistency. We don't memoize mergeClasses() in any of the existing components (afaik), and I have minor concerns about things slipping through the cracks when we get to refactoring classify one day.

[sirugh]

Thanks for fixing the validator! Sucks we had to do a one-off but hopefully we can figure out a long term solution for client-side schema at a later time :)

[dpatil-magento]

@revanth0212 I can see gift options even after disabling this setting in Admin UI. Is this expected?

[sirugh]

@revanth0212 I can see gift options even after disabling this setting in Admin UI. Is this expected?

Oh man I didn't even realize this was a thing. I think that we should create a fast-follow that goes back and enables/disables cart features based on their admin toggles. This will probably require either some build or runtime GQL check.