fix: handle tagline field on ncps hosted buttons #2384
Conversation
| @@ -97,6 +97,7 @@ export const getHostedButtonDetails: HostedButtonDetailsParams = async ({ | |||
| shape: getButtonVariable(variables, "shape"), | |||
| color: getButtonVariable(variables, "color"), | |||
| label: getButtonVariable(variables, "button_text"), | |||
| tagline: getButtonVariable(variables, "tagline") === "true", | |||
There was a problem hiding this comment.
Seeing this QUERY_BOOL constant in sdk-contants. Not sure if this is an appropriate case to use that.
| // $FlowIssue | ||
| request.mockImplementationOnce(() => | ||
| // eslint-disable-next-line compat/compat | ||
| Promise.resolve({ |
There was a problem hiding this comment.
can this reuse the previous response from
?There was a problem hiding this comment.
Yeah, thought about it, but the tests are changing the value for tagline (true, false, undefined). Felt a new object was cleaner than nested spreads and mapping over the array to change one value.
src/hosted-buttons/index.test.js
Outdated
| ); | ||
| await HostedButtons({ | ||
| hostedButtonId: "B1234567890", | ||
| fundingSources: ["paypal", "venmo"], |
There was a problem hiding this comment.
I think fundingSources can be removed here
There was a problem hiding this comment.
Yeah, I would remove these. This is how we are temporarily rendering the v2 experience but will remove the fundingSources in this ticket that's currently in the sprint.
The tagline only gets rendered in the horizontal smart stack and the v2 experience is rendering 2 standalone buttons.
| ); | ||
| expect(Buttons).toHaveBeenCalledTimes(2); | ||
| expect(renderMock).toHaveBeenCalledTimes(0); | ||
| expect.assertions(3); |
| { | ||
| name: "button_text", | ||
| value: "paypal", | ||
| }, |
There was a problem hiding this comment.
^ similar comment about reusing the previous response variables
src/hosted-buttons/utils.test.js
Outdated
| ); | ||
| await getHostedButtonDetails({ | ||
| hostedButtonId, | ||
| fundingSources: [], |
There was a problem hiding this comment.
I think fundingSources can be removed here
There was a problem hiding this comment.
Yeah, I would remove these. This is how we are temporarily rendering the v2 experience but will remove the fundingSources in this ticket that's currently in the sprint.
The tagline only gets rendered in the horizontal smart stack and the v2 experience is rendering 2 standalone buttons.
There was a problem hiding this comment.
It's currently required on the type. Are we removing it completely in that ticket? Might make it optional for now, and @nbierdeman can address in that ticket?
There are other tests that included this field.
Description
Hosted Buttons are not able to disable the
taglineon horizontal layout buttons. This change uses thetaglinevalue from/ncp/api/form-fields. Defaulting tofalseif the field is not provided.Why are we making these changes?
Bug fix: LI-45315
Screenshots
Before:
With fix:
