feat: reserves space for button message and renders messageMarkup #2335

danzhaaspaypal · 2024-02-14T18:15:41Z

Description

For the Buttons dynamic messaging feature:
In the Buttons component, reserves space for button message during 1st render and renders messageMarkup on second render.

Why are we making these changes? Include references to any related Jira tasks or GitHub Issues

Dynamic Messaging with Buttons Epic and Update button render to reserve message space Story

Reproduction Steps (if applicable)

Pass in message prop to button component, optionally specifying its own child property position with 'top' or 'bottom'. Conditionally, a 36 px space will populate above or below the group of buttons.

Screenshots (if applicable)

Dependent Changes (if applicable)

n/a

Groups who should review (if applicable)

Upstream Messaging
XO components

❤️ Thank you!

src/zoid/buttons/component.jsx

src/ui/buttons/message.jsx

src/ui/buttons/buttons.jsx

src/ui/buttons/message.jsx

src/ui/buttons/buttons.jsx

test/integration/tests/button/message.js

src/ui/buttons/buttons.jsx

test/integration/tests/button/message.js

src/ui/buttons/buttons.jsx

…/paypal-checkout-components into feature/btn-msg-space1

codecov · 2024-02-21T19:53:00Z

Codecov Report

Attention: Patch coverage is 58.06452% with 13 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (feature/dynamic-messaging@0806df1). Click here to learn what that means.

Files	Patch %	Lines
src/ui/buttons/buttons.jsx	61.53%	5 Missing ⚠️
src/ui/buttons/util.js	58.33%	5 Missing ⚠️
src/ui/buttons/message.jsx	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@                     Coverage Diff                      @@
##             feature/dynamic-messaging    #2335   +/-   ##
============================================================
  Coverage                             ?   52.20%           
============================================================
  Files                                ?      106           
  Lines                                ?     2109           
  Branches                             ?      641           
============================================================
  Hits                                 ?     1101           
  Misses                               ?      904           
  Partials                             ?      104

Flag	Coverage Δ
jest	`32.03% <35.48%> (?)`
karma	`51.25% <75.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wsbrunson · 2024-02-22T15:08:21Z

src/ui/buttons/message.jsx

+  const reservationDiv = `<div class="${CLASS.BUTTON_MESSAGE_RESERVE}" style="height:${INITIAL_RESERVED_HEIGHT}" ></div>`;
+
+  return (
+    <div
+      class={[CLASS.BUTTON_MESSAGE, `${CLASS.BUTTON_MESSAGE}-${position}`].join(
+        " "
+      )}
+      innerHTML={typeof markup === "string" ? markup : reservationDiv}
+    />
+  );


Github UI probably won't format this well but I think something like this would be clearer in intention:

Suggested change

const reservationDiv = `<div class="${CLASS.BUTTON_MESSAGE_RESERVE}" style="height:${INITIAL_RESERVED_HEIGHT}" ></div>`;

return (

<div

class={[CLASS.BUTTON_MESSAGE, `${CLASS.BUTTON_MESSAGE}-${position}`].join(

" "

)}

innerHTML={typeof markup === "string" ? markup : reservationDiv}

/>

);

const messageClassNames = [CLASS.BUTTON_MESSAGE, `${CLASS.BUTTON_MESSAGE}-${position}`].join(" ")

if (!typeof markup === "string") {

return <div className={messageClassNames}>

<div className={CLASS.BUTTON_MESSAGE_RESERVE} style={`height:${INITIAL_RESERVED_HEIGHT}`} />

</div>

}

return (

<div

className={messageClassNames}

innerHTML={markup}

/>

);

or you could do something like this:

Suggested change

const reservationDiv = `<div class="${CLASS.BUTTON_MESSAGE_RESERVE}" style="height:${INITIAL_RESERVED_HEIGHT}" ></div>`;

return (

<div

class={[CLASS.BUTTON_MESSAGE, `${CLASS.BUTTON_MESSAGE}-${position}`].join(

" "

)}

innerHTML={typeof markup === "string" ? markup : reservationDiv}

/>

);

const messageClassNames = [CLASS.BUTTON_MESSAGE, `${CLASS.BUTTON_MESSAGE}-${position}`].join(" ")

if (!typeof markup === "string") {

return <div className={`${messageClassNames} $CLASS.BUTTON_MESSAGE_RESERVE}`} style={`height:${INITIAL_RESERVED_HEIGHT}`} />

}

return (

<div

className={messageClassNames}

innerHTML={markup}

/>

);

also also className -> class in all my examples i forgot i'm not writing React 😂

jshawl

I added a few non-blocking comments but LGTM 🚀

jshawl · 2024-02-22T15:00:10Z

src/ui/buttons/buttons.jsx

+    return MESSAGE_POSITION.TOP;
+  }
+  return MESSAGE_POSITION.BOTTOM;
+}


option to move this function ui/buttons/message.jsx (I did not see it used elsewhere and/or reference variables in the parent scope)

I like the idea of decluttering buttons.jsx; I'd like to suggest ui/buttons/util.js as another good place to consider moving that function. Thoughts, preference?

I think ui/buttons/util.js would work well! 🚀

jshawl · 2024-02-22T15:04:54Z

src/ui/buttons/buttons.jsx

@@ -150,6 +152,28 @@ export function validateButtonProps(props: ButtonPropsInputs) {
  normalizeButtonProps(props);
 }

+function calculateMessagePosition({ message, showPoweredBy, layout }): string {
+  if (!message) {
+    return "none";


option to add a NONE constant here -

paypal-checkout-components/src/constants/button.js

Lines 77 to 80 in fbe7381

export const MESSAGE_POSITION = {

TOP: ("top": "top"),

BOTTOM: ("bottom": "bottom"),

};

(for consistency) and change the return type to $Values<typeof MESSAGE_POSITION> (just kind of cool)

We do not want NONE to be considered a valid input option which MESSAGE_POSITION is currently used for as a type. It's purely an internal computed value. Would it be beneficial to have a separate constant to capture the possible internal values?

ah that's a good point! I think if it were me I would leave it as-is. one other idea just for fun would be to make the return type string | void and the early return would return nothing/undefined. But I think I like what you have here better than that

@jshawl Nate and I tried a few different approaches being mindful of type returned here; the null return approach was our first attempted strategy, and if Flow would be mindful of our type logic, we would gladly go with that option. But it didn't exactly mind our logic correctly: it tried to suggest null should be a possible value in the Message component where it cannot logically be passed in due to logic on lines 302 and 359. And it seemed wrong to abide that workaround by accommodating null inside Message where it could not appear.

We tried a few other typing workarounds, and finally settled on this being the least of the evils.

this is flow's fault 😂

jshawl · 2024-02-22T15:13:55Z

src/ui/buttons/buttons.jsx

+    !fundingSource &&
+    !message;
+  const showPoweredBy =
+    layout === BUTTON_LAYOUT.VERTICAL && fundingSources.includes(FUNDING.CARD);


I think fundingSources will always include FUNDING.CARD -

paypal-checkout-components/src/ui/buttons/buttons.jsx

Lines 240 to 245 in fbe7381

if (fundingSources.indexOf(FUNDING.CARD) !== -1) {

fundingSources = [

...fundingSources.filter((src) => src !== FUNDING.CARD),

FUNDING.CARD,

];

}

(so can remove the && fundingSources.includes)

I actually don't think it would. Its confusing, but the standalone button case uses Buttons component but with fundingSource=paypal|venmo|card|etc. I think this check is precisely for those instances where we are in Standalone and its not the Card button

ah good catch! thank you for clarifying that!

Not a problem, actually that would be a good candidate for a refactor. It would probably simplify things if we created a StandaloneButton component and rendered that instead of Buttons if fundingSource was passed

jshawl · 2024-02-22T15:14:41Z

test/integration/tests/button/message.js

+      });
+    });
+  });
+});


awesome test coverage!! 💯 🏅

@jshawl thank you!

reserves space for message and renders messageMarkup

84468e3

danzhaaspaypal requested a review from Seavenly February 14, 2024 18:16

Seavenly reviewed Feb 14, 2024

View reviewed changes

fine tunes and adds integration tests

d4407d2

Seavenly reviewed Feb 16, 2024

View reviewed changes

src/ui/buttons/message.jsx Outdated Show resolved Hide resolved

src/ui/buttons/message.jsx Outdated Show resolved Hide resolved

updates logic & cleans code

47550d4

Seavenly reviewed Feb 16, 2024

View reviewed changes

updates logic for when there is no message to accomodate typecheck

e4a9728

Seavenly reviewed Feb 21, 2024

View reviewed changes

src/ui/buttons/buttons.jsx Outdated Show resolved Hide resolved

src/ui/buttons/buttons.jsx Outdated Show resolved Hide resolved

danzhaaspaypal added 2 commits February 21, 2024 11:58

corrects message component prop type and logic

0ffcaa6

Merge branch 'feature/dynamic-messaging' of https://github.com/paypal…

fbe7381

…/paypal-checkout-components into feature/btn-msg-space1

Seavenly approved these changes Feb 21, 2024

View reviewed changes

Seavenly marked this pull request as ready for review February 21, 2024 20:48

Seavenly requested a review from a team as a code owner February 21, 2024 20:48

wsbrunson approved these changes Feb 22, 2024

View reviewed changes

jshawl approved these changes Feb 22, 2024

View reviewed changes

danzhaaspaypal added 2 commits February 22, 2024 16:17

cleans code per GH comments

adc3e9c

adds types where needed

f4a485c

Seavenly merged commit 11989d5 into feature/dynamic-messaging Feb 23, 2024
1 of 3 checks passed

Seavenly deleted the feature/btn-msg-space1 branch February 23, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reserves space for button message and renders messageMarkup #2335

feat: reserves space for button message and renders messageMarkup #2335

danzhaaspaypal commented Feb 14, 2024

codecov bot commented Feb 21, 2024 •

edited

wsbrunson Feb 22, 2024

wsbrunson Feb 22, 2024

wsbrunson Feb 22, 2024

danzhaaspaypal Feb 22, 2024

jshawl left a comment

jshawl Feb 22, 2024

danzhaaspaypal Feb 22, 2024

jshawl Feb 22, 2024

jshawl Feb 22, 2024 •

edited

Seavenly Feb 22, 2024

jshawl Feb 22, 2024

danzhaaspaypal Feb 22, 2024 •

edited

jshawl Feb 22, 2024

danzhaaspaypal Feb 22, 2024

jshawl Feb 22, 2024

wsbrunson Feb 22, 2024

jshawl Feb 22, 2024

wsbrunson Feb 22, 2024

jshawl Feb 22, 2024

danzhaaspaypal Feb 22, 2024

-  const reservationDiv = `<div class="${CLASS.BUTTON_MESSAGE_RESERVE}" style="height:${INITIAL_RESERVED_HEIGHT}" ></div>`;
-  return (
-    <div
-      class={[CLASS.BUTTON_MESSAGE, `${CLASS.BUTTON_MESSAGE}-${position}`].join(
-        " "
-      )}
-      innerHTML={typeof markup === "string" ? markup : reservationDiv}
-    />
-  );
+  const messageClassNames = [CLASS.BUTTON_MESSAGE, `${CLASS.BUTTON_MESSAGE}-${position}`].join(" ")
+  if (!typeof markup === "string") {
+    return <div className={messageClassNames}>
+      <div className={CLASS.BUTTON_MESSAGE_RESERVE} style={`height:${INITIAL_RESERVED_HEIGHT}`} />
+     </div>
+  }
+  return (
+    <div
+      className={messageClassNames}
+      innerHTML={markup}
+    />
+  );

	export const MESSAGE_POSITION = {
	TOP: ("top": "top"),
	BOTTOM: ("bottom": "bottom"),
	};

	if (fundingSources.indexOf(FUNDING.CARD) !== -1) {
	fundingSources = [
	...fundingSources.filter((src) => src !== FUNDING.CARD),
	FUNDING.CARD,
	];
	}

feat: reserves space for button message and renders messageMarkup #2335

feat: reserves space for button message and renders messageMarkup #2335

Conversation

danzhaaspaypal commented Feb 14, 2024

Description

Why are we making these changes? Include references to any related Jira tasks or GitHub Issues

Reproduction Steps (if applicable)

Screenshots (if applicable)

Dependent Changes (if applicable)

Groups who should review (if applicable)

codecov bot commented Feb 21, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshawl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshawl Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzhaaspaypal Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2024 •

edited

jshawl Feb 22, 2024 •

edited

danzhaaspaypal Feb 22, 2024 •

edited