New Feature: disableMaxHeight #2361
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2361 +/- ##
==========================================
+ Coverage 51.32% 51.96% +0.63%
==========================================
Files 104 148 +44
Lines 2038 11986 +9948
Branches 604 679 +75
==========================================
+ Hits 1046 6228 +5182
- Misses 889 5653 +4764
- Partials 103 105 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
nikrom17
changed the title
nikrom17
requested review from
wsbrunson,
jshawl,
ravishekhar,
nbierdeman,
spencersablan and
gregjopa
|
|
||
| // $FlowFixMe | ||
| ].map(({ height, size, valid }) => ({ | ||
| ].map(({ height, valid, disableMaxHeight }) => ({ |
There was a problem hiding this comment.
It didn't seem like size was used at all in this sections so I removed it and used the sentence structure to indicate if the disbaleMaxHeight feature was enabled.
src/ui/buttons/props.js
Outdated
| } | ||
|
|
||
| const disableMaxHeightInvalidFundingSources = [FUNDING.CARD, undefined]; | ||
| const disableMaxHeightValidFundingSources = [ |
There was a problem hiding this comment.
why are these the only ones supported for disabling max height?
There was a problem hiding this comment.
The only 2 buttons that aren't supported are the smart stack and card. The main reason these aren't supported was the challenge of getting them to render correctly and not being the specific use case that Stripe was looking for.
It seemed like they were rendering the standalone paypal button so we focused on enabling that only.
The card button is not supported because the inline guest experience gets rendered in the same container as the card button. If the merchant but a height limitation on the container for the card button, then it was also going to apply to the inline guest experience.
I know we have a bunch of other funding sources, but I believe we only have buttons for the funding sources below. The other funding sources are for the card fields, correct?
There was a problem hiding this comment.
We also have all the APMs although maybe we don't support them as standalone buttons? If so, I would think we would have a config value somewhere that disallows those buttons as standalone
https://github.com/paypal/paypal-sdk-constants/blob/main/src/funding.js#L3
There was a problem hiding this comment.
Looks like all those funding sources can be rendered as a standalone button. I've updated the error message to include all of those as well.
There was a problem hiding this comment.
I would consider the following tweaks, might be easier to maintain:
-
Early return if
fundingSource === undefined. Error message could be something likedisableMaxHeight is only supported for standalone buttons. You can render a standalone button by passingfundingSourceto theButton` config: paypal.Buttons({fundingSource: paypal.FUNDING.PAYPAL}) -
Then, down below, throw the error for invalid buttons and just say "This funding source, {fundingSource}, does not support
disableMaxHeight"
That's some very rough copy, feel free to change it if you decide to go in this direction
|
Thanks to
|
Description
Stripe wants the paypal button to expand to fill it's parent container. This PR added a new style prop
disableMaxHeightthat remove all height limitations on the button and adds cssheight: 100%everywhere.Why are we making these changes? Include references to any related Jira tasks or GitHub Issues
Jira Ticket
Feature Branch Deployed to Test Env
I've deployed this feature branch to a test env.
I've also hooked up that test env to a storybook with a few examples of how a merchant could structure their html to showcase how the button would render in those scenarios.
https://github.paypal.com/pages/Core-SDK/button-redesign-storybook/?path=/story/edge-cases-button-disablemaxheight--container-with-fixed-height
Testing Issues
Snapshot testing seems to be the best way to go about testing this new feature, however the Percy Snapshot tests seems broken on this repo. I've spend some time trying to fix, opened a ticket with Browserstack and still havent be able to triage the exact issue for why they are failing.
I wanted to get the PR out there so yall can look at the code and then start a discussion on how we want to handle the testing.
I did update the validation tests in test/integration/tests/button/validation.js
Differences in button rendering between using
style.heightanddisbableMaxHeightThe responsive styles of the PayPal word mark and label are calculated based off a specific height value. This comes from our default settings (height increases as width increases) or the
style.heightproperty.I discussed this heavily with product and we decided to go this route since this solution was faster to implement and we could always go back and refactor the responsive styles if necessiary.
Below showcases the different ways the button will render based on if you specify the
style.heightproperty.Groups who should review (if applicable)
❤️ Thank you!