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

Applied Gift Cards show current balance #2156

Merged
merged 5 commits into from
Feb 12, 2020

Conversation

supernova-at
Copy link
Contributor

@supernova-at supernova-at commented Feb 10, 2020

Description

This PR adds current applied gift card balances to the UI to improve the UX.

The balance font size is slightly smaller than the code, so I consulted the Venia Styleguide for its value.

Related Issue

Closes PWA-364.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Go to the /cart page
  2. Apply a gift card or two
  3. See that the card's balance appears along with its code

Screenshots / Screen Captures (if appropriate)

Screen Shot 2020-02-10 at 12 22 20 PM

Checklist

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

@supernova-at supernova-at changed the title Supernova/364 gift card balance Applied Gift Cards show current balance Feb 10, 2020
}

.balance {
font-size: 0.875rem;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is computed based on the Venia Styleguide:

Screen Shot 2020-02-10 at 11 52 40 AM

Since we don't have CSS Properties (variables) for font sizes yet, 0.875rem = 14px.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Feb 10, 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-364.

Generated by 🚫 dangerJS against af7baf6

unit test card code
</span>
<span>
Balance:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no balance in these snaps, not even a zero. Expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expected because the value is rendered via the Price component, which gets mocked here to an empty <div /> (see next line).

sirugh
sirugh previously approved these changes Feb 11, 2020
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.

Aside from the snaps (maybe) being off, I approve!

@schensley will want to approve as well, methinks.

@sirugh sirugh added the version: Patch This changeset includes backwards compatible bug fixes. label Feb 11, 2020
@schensley
Copy link

@supernova-at the implementation looks very good. One fussy thing, the balance could use a little bit more separation for the card number once it has been applied.

<span>
00
</span>
</span>
Copy link
Contributor Author

@supernova-at supernova-at Feb 12, 2020

Choose a reason for hiding this comment

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

@sirugh I think this Price component doesn't get mocked because we requireActual Peregrine in this .spec.js. Apparently that will ignore mocks in __mocks__ 🤷‍♂

@supernova-at
Copy link
Contributor Author

@supernova-at the implementation looks very good. One fussy thing, the balance could use a little bit more separation for the card number once it has been applied.

@schensley , resolved by af7baf6.

Copy link

@schensley schensley left a comment

Choose a reason for hiding this comment

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

UX approved.
Thnx.

@dpatil-magento dpatil-magento merged commit 82b9e59 into develop Feb 12, 2020
@dpatil-magento dpatil-magento deleted the supernova/364_gift_card_balance branch February 12, 2020 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-ui Progress: review version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants