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

PAYMENTS-1160: Ensure Apple Pay button is hidden for incompatible browsers. #1084

Merged
merged 1 commit into from
Sep 6, 2017
Merged

PAYMENTS-1160: Ensure Apple Pay button is hidden for incompatible browsers. #1084

merged 1 commit into from
Sep 6, 2017

Conversation

PascalZajac
Copy link
Contributor

@PascalZajac PascalZajac commented Sep 4, 2017

What?

Remove an unnecessary display: block rule that was causing the Apple Pay button to appear in the Cart Modal even for browsers that do not support Apple Pay.

Why?

Shopper confusion. The rule in question is ignoring the apple-pay-supported class that gets attached to the body element, forcing the button to appear in all browsers. My first solution was to fix this by changing the nesting of the rules to respect this class, but then I realised that the display: block rule itself is unnecessary, because it will be inherited from the rule defined on L33.

Screenshots (if appropriate)

Since I can't enable Apple Pay for local development, I have screenshots showing the HTML structure in Chrome and Safari respectively to prove that this should work.
screen shot 2017-09-04 at 3 53 42 pm
screen shot 2017-09-04 at 3 53 21 pm

Ping @bigcommerce/stencil-team @bigcommerce/payments

poweredbyanna
poweredbyanna previously approved these changes Sep 5, 2017
Copy link
Contributor

@poweredbyanna poweredbyanna left a comment

Choose a reason for hiding this comment

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

Seems legit but you've made Travis unhappy

@davidchin
Copy link
Contributor

It seems you have to apply // scss-lint:disable SelectorDepth, NestingDepth (unless you want to refactor the CSS, but I guess it's a bit difficult for you because you are not able to turn on Apple Pay locally so I'm happy for you to just fix the bug 😄)

@poweredbyanna
Copy link
Contributor

@philipmuir may be able to help you test it locally if you need (and then you can both teach us!)

@PascalZajac
Copy link
Contributor Author

Thanks to Travis' prompting, I decided to take a different approach to this, and instead remove the display: block rule from the previewCartCheckout ruleset, which means display will just be inherited from the rule defined above. I updated the PR description accordingly.

@PascalZajac PascalZajac dismissed poweredbyanna’s stale review September 5, 2017 14:52

Dismissing due to changed implementation.

@junedkazi
Copy link
Contributor

@PascalZajac can you add a changelog entry before you merge this one.

@PascalZajac
Copy link
Contributor Author

Sure thing, under the Draft heading right?

@junedkazi
Copy link
Contributor

Yes it should be in the draft section.

Copy link
Contributor

@davidchin davidchin left a comment

Choose a reason for hiding this comment

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

Nice, thanks @PascalZajac for making the change, LGTM!

@PascalZajac PascalZajac merged commit 96a1764 into bigcommerce:master Sep 6, 2017
@PascalZajac PascalZajac deleted the PAYMENTS-1160 branch September 6, 2017 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants