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

Jump to bottom

Peregrine implementation/receipt into checkout slice #1686

Merged
merged 3 commits into from
Sep 17, 2019
Merged

Peregrine implementation/receipt into checkout slice #1686

merged 3 commits into from
Sep 17, 2019

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Sep 13, 2019

Description

The checkoutReceipt slice doesn't really need to exist separate from checkout.

Related Issue

Closes #1685 .

Verification Steps

  1. Work through checkout and make sure the receipt opens.
  2. On the receipt page, click the X, and then reopen the cart. It should show the empty cart view.
  3. Work through checkout again and this time click 'create account' on receipt. It should navigate to the create account page. Reopen the cart, it should show an empty cart.
  4. Work through checkout again and this time click the mask (left of the cart). It should close the cart. Reopen the cart and it should show the empty cart.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@sirugh sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Sep 13, 2019
@sirugh sirugh changed the base branch from develop to feature/peregrine-implementation September 13, 2019 17:53
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Sep 13, 2019 •
edited
Loading

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

Generated by 🚫 dangerJS against a028628

@supernova-at supernova-at self-assigned this Sep 13, 2019
supernova-at
supernova-at requested changes Sep 13, 2019
View reviewed changes
Copy link
Contributor

@supernova-at supernova-at left a comment

Choose a reason for hiding this comment

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

Love to see stuff like this. Two very minor things then 👍

...e/lib/PeregrineContextProvider/__tests__/__snapshots__/peregrineContextProvider.spec.js.snap Outdated
<Memo(ConnectFunction)>
<Memo(ConnectFunction)>
<Memo(ConnectFunction)>
<Memo(ConnectFunction)>
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels nitpicky but snapshots like this aren't very useful, lol. Maybe if we put the number in the test name? "...wraps children with all six context providers" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it's not helpful. The only reason this is changing is because feature branch was broken but your complaint, at it's core, is very relevant. We should delete this or assert on the number of contexts being provided or something...

packages/venia-ui/lib/components/Checkout/Receipt/receipt.js

const classes = mergeClasses(defaultClasses, props.classes);

useEffect(() => reset, [reset]);
// When the drawer is closed reset the state of the receipt. We use a ref
// because drawer can change if the mask is clicked. Mask updates drawer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own education, what's the benefit of using useRef over useState here?

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 don't want to trigger a re-render.

@sirugh
Copy link
Contributor Author

sirugh commented Sep 13, 2019

@supernova-at addressed feedback -- going to wait until the feature branch is merged and then change this base, just FYI.

supernova-at
supernova-at previously approved these changes Sep 16, 2019
View reviewed changes
@supernova-at supernova-at removed their assignment Sep 16, 2019
@supernova-at supernova-at added the hold On hold until another condition is fulfilled. label Sep 16, 2019
@sirugh sirugh changed the base branch from feature/peregrine-implementation to develop September 16, 2019 19:19
@sirugh sirugh changed the base branch from develop to feature/peregrine-implementation September 16, 2019 19:20
@sirugh sirugh force-pushed the peregrine-implementation/receipt-into-checkout-slice branch 2 times, most recently from b5709dd to 352f925 Compare September 16, 2019 19:23
@sirugh sirugh force-pushed the peregrine-implementation/receipt-into-checkout-slice branch from 352f925 to 40ab163 Compare September 16, 2019 19:25
@sirugh sirugh changed the base branch from feature/peregrine-implementation to develop September 16, 2019 19:25
@sirugh sirugh removed the hold On hold until another condition is fulfilled. label Sep 16, 2019
@sirugh sirugh mentioned this pull request Sep 17, 2019
Merged
@dpatil-magento
Copy link
Contributor

dpatil-magento commented Sep 17, 2019 •
edited
Loading

@sirugh Chrome desktop view, clear browser cache, Access Venia > Add product to cart and try closing Shopping cart by click X icon. Icon looks dead.
Tested on - https://pr-1686.pwa-venia-staging.com
image

@sirugh
Copy link
Contributor Author

sirugh commented Sep 17, 2019

Spoke with @dpatil-magento and the issue he saw is unrelated.

@dpatil-magento dpatil-magento merged commit 116edb2 into develop Sep 17, 2019
@dpatil-magento dpatil-magento deleted the peregrine-implementation/receipt-into-checkout-slice branch September 17, 2019 19:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@supernova-at supernova-at supernova-at approved these changes

Assignees
No one assigned
Labels
pkg:peregrine pkg:venia-ui version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Milestone
Modular Component Library (Phase 1)
Development

Successfully merging this pull request may close these issues.

Merge checkoutReceipt actions/state into checkout
4 participants
@sirugh @PWAStudioBot @dpatil-magento @supernova-at