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.

Sign up for GitHub

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

Jump to bottom

PWA-560: Fix gift card flash on error #2462

Merged
merged 10 commits into from
Jun 11, 2020
Merged

PWA-560: Fix gift card flash on error #2462

merged 10 commits into from
Jun 11, 2020

Conversation

davemacaulay
Copy link
Contributor

@davemacaulay davemacaulay commented Jun 5, 2020 •
edited

Description

Maintain state for whether or not to show the gift card error so it can be correctly updated based on other state in the gift card talon.

Related Issue

https://jira.corp.magento.com/browse/PWA-560

Acceptance

Verification Stakeholders

@jimbo
@dhaecker
@sirugh

Specification

Verification Steps

  1. Add a card
  2. Enter the same card and click "Apply Card". You'll get an error.
  3. Click "Remove" to remove the original card. The error should disappear.
  4. Click "Apply Card". You will not see a flash of error between states.
    See JIRA ticket for video of issue.

Screenshots / Screen Captures (if appropriate)

See JIRA ticket for video

Checklist

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

@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Jun 5, 2020
@davemacaulay davemacaulay added the version: Patch This changeset includes backwards compatible bug fixes. label Jun 5, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jun 5, 2020 •
edited

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

Generated by 🚫 dangerJS against 2b0685d

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented Jun 5, 2020 •
edited

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2462.pwa-venia.com : LH Performance Expected 0.85 Actual 0.55, LH Best Practices Expected 1 Actual 0.92
https://pr-2462.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.35, LH Best Practices Expected 1 Actual 0.92
https://pr-2462.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.5, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

@m2-community-project m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Jun 5, 2020
@davemacaulay
PWA-560: Fix gift card flash on error
6263d21 
- Refactor Talon to improve code readability
@davemacaulay
PWA-560: Fix gift card flash on error
67c39ef 
- Add test to verify that calling applyGiftCard with an error in the response will return shouldDisplayCardError true
@davemacaulay
PWA-560: Fix gift card flash on error
277a3aa 
- Resolve prettier and lint issues
@davemacaulay
Merge branch 'develop' into PWA-560
0515382
sirugh
sirugh previously approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Good job :) And thank you for writing a test to cover the change.

@m2-community-project m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Jun 9, 2020
tjwiebell
tjwiebell suggested changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Looks good. One required suggestion, a couple related to memos that probably are over-opts. Nice work 👍

packages/peregrine/lib/talons/CartPage/GiftCards/useGiftCards.js Outdated
formApi.reset();
}, [formApi, applyCard, cartId]);

const checkGiftCardBalance = useCallback(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like this needs to be a Promise anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, thank you!

packages/peregrine/lib/talons/CartPage/GiftCards/useGiftCards.js Outdated
@@ -197,6 +150,21 @@ export const useGiftCards = props => {
setIsCartUpdating
]);

const shouldDisplayCardBalance = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect this memo to be calculated every time, not sure this is an improvement; what was wrong with the current implementation of derived state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We went on an adventure fixing this bug, it seems the my refactoring to the rest of the talon resolved the original error, so you're correct. The useMemo is no longer needed, I've updated the code :)

packages/peregrine/lib/talons/CartPage/GiftCards/useGiftCards.js Outdated
);

// We should only display the last card error if the most recent action was apply or check and they had an error
const shouldDisplayCardError = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here; why the memos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2462 (comment)

@m2-community-project m2-community-project bot moved this from Reviewer Approved to Changes Requested in Pull Request Progress Jun 9, 2020
@davemacaulay
PWA-560: Fix gift card flash on error
d256f93 
- Remove usage of useMemo and use derived state
tjwiebell
tjwiebell previously approved these changes Jun 9, 2020
View reviewed changes
@sirugh
Copy link
Contributor

sirugh commented Jun 9, 2020

You need to remove the useMemo import as well, I think. Linter is complaining.

sirugh
sirugh approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Jinx

@m2-community-project m2-community-project bot moved this from Changes Requested to Reviewer Approved in Pull Request Progress Jun 9, 2020
@dhaecker
Copy link
Collaborator

QA approved

@davemacaulay davemacaulay merged commit 677a7dc into magento:develop Jun 11, 2020
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Jun 11, 2020
@davemacaulay davemacaulay deleted the PWA-560 branch June 11, 2020 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revanth0212 revanth0212 revanth0212 left review comments

@tjwiebell tjwiebell tjwiebell approved these changes

@sirugh sirugh sirugh approved these changes

@jimbo jimbo jimbo approved these changes

Assignees

@tjwiebell tjwiebell

@sirugh sirugh

@jimbo jimbo

@revanth0212 revanth0212

Labels
pkg:peregrine pkg:venia-ui version: Patch This changeset includes backwards compatible bug fixes.
Projects
Pull Request Progress
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@davemacaulay @PWAStudioBot @devops-pwa-codebuild @sirugh @dhaecker @tjwiebell @jimbo @revanth0212