-
Notifications
You must be signed in to change notification settings - Fork 27
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
ASUB-7844 PayPal block #1806
ASUB-7844 PayPal block #1806
Conversation
@@ -1,17 +1,22 @@ | |||
import React, { useEffect, useState } from "react"; | |||
import React, { useEffect, useState, useMemo } from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [stylelint] reported by reviewdog 🐶
Unknown word (CssSyntaxError) CssSyntaxError
@@ -0,0 +1,105 @@ | |||
import { useState, useEffect } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [stylelint] reported by reviewdog 🐶
Unknown word (CssSyntaxError) CssSyntaxError
@@ -1,4 +1,4 @@ | |||
import React, { useEffect, useState } from "react"; | |||
import React, { useEffect, useState, useMemo } from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [stylelint] reported by reviewdog 🐶
Unknown word (CssSyntaxError) CssSyntaxError
blocks/subscriptions-block/features/checkout/_children/PaymentInfo/index.jsx
Show resolved
Hide resolved
@@ -50,6 +50,11 @@ | |||
@include scss.block-components("checkout-payment"); | |||
@include scss.block-properties("checkout-payment"); | |||
|
|||
&-info { | |||
@include scss.block-compoents("checkout-payment-info"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 21,22, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 116,117,118, Chrome 114,115,116,117,118, Safari 16.6,17.0, Opera 101,102,103, Safari on iOS 16.6,17.0, Android Browser 118, Chrome for Android 118 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
@@ -0,0 +1,66 @@ | |||
import { useState, useEffect } from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 [stylelint] reported by reviewdog 🐶
Unknown word (CssSyntaxError) CssSyntaxError
|
||
useEffect(() => { | ||
const initPayment = async () => { | ||
const config = await Sales.getConfig(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwardcho1231 config
is not being used, you can remove this
setError(e); | ||
} | ||
}; | ||
if (currentOrder && currentMerchantId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can modify this by
if (currentOrder?.orderNumber && currentMerchantId),
and delete if
on L37
@@ -1,51 +1,174 @@ | |||
import React, { useEffect, useState } from "react"; | |||
import React, { useEffect, useState, useMemo } from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useMemo
is unused
const payWithCardDividerLabel = phrases.t("subscriptions-block.payWithCard-label"); | ||
|
||
useEffect(() => { | ||
if (stripeIntents?.paymentMethodID && !errorPaymentOption) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we need to add !isInitialized
.
if (stripeIntents?.paymentMethodID && !errorPaymentOption && !isInitialized)
, in order to avoid to call get and create a new cart, if we are trying to call finalize
import PropTypes from "@arc-fusion/prop-types"; | ||
import { usePhrases, Heading, Link, useIdentity } from "@wpmedia/arc-themes-components"; | ||
import useSales from "../../components/useSales"; | ||
import usePaymentOptions from "../../components/usePaymentOptions"; | ||
import Cart from "../../components/Cart"; | ||
import ContactInfo from "../../components/ContactInfo"; | ||
import PaymentInfo from "./_children/PaymentInfo"; | ||
|
||
const BLOCK_CLASS_NAME = "b-checkout"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since LABEL_ORDER_NUMBER_PAYPAL it's a constant, maybe define here at the beginig
const finalizePayment = async () => { | ||
try { | ||
await Sales.finalizePayment(orderNumber, currentMerchantId, token, null); | ||
window.location.href = successURL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the order from localstorage, once this call is done
locale/en.json
Outdated
"subscriptions-block.payment-information": "Credit card information", | ||
"subscriptions-block.submit-payment": "Purchase Subscription" | ||
"subscriptions-block.submit-payment": "Purchase Subscription", | ||
"subscriptions-block.payWithCard-label": "Or Pay with card" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwardcho1231 , when adding a new translation key, we need it added for all languages. The guide for doing so is here: https://arcpublishing.atlassian.net/wiki/spaces/TI/pages/2538275032/Arc+Themes+Blocks+Internationalisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vgalatro I thought you guys put in the lokalise order. Is this not true and we should put in the order for translation and commit the change to this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to put in the order, just update the data we have in Lokalise so our product owner can make the order later.
Although thinking about this change a bit more, is it really necessary to change the already existing string right now, from subscriptions-block.payment-information
to subscriptions-block.payWithCard-label
? We have a lot in progress with our RTL work and adding new translation strings at the moment is not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have access to Lokalise themes project so I cannot upload files into the project. We are not changing an existing string but adding a new one as it's requested by the design.
<HeadingSection> | ||
<Heading>{formLabel}</Heading> | ||
</HeadingSection> | ||
{paypal && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since here, L135 & L144 you are checking paypal exists, maybe you can include all these in a single block, something like
{paypal && ( .... "all you have related to paypal" )}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need two blocks since we want the divider to show even when isPayPal
is false
(before the user clicks on the PayPal button)
@@ -50,6 +50,41 @@ | |||
@include scss.block-components("checkout-payment"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
@@ -50,6 +50,41 @@ | |||
@include scss.block-components("checkout-payment"); | |||
@include scss.block-properties("checkout-payment"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
@@ -50,6 +50,41 @@ | |||
@include scss.block-components("checkout-payment"); | |||
@include scss.block-properties("checkout-payment"); | |||
|
|||
&-info { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
@@ -50,6 +50,41 @@ | |||
@include scss.block-components("checkout-payment"); | |||
@include scss.block-properties("checkout-payment"); | |||
|
|||
&-info { | |||
@include scss.block-components("checkout-payment-info"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
@@ -50,6 +50,41 @@ | |||
@include scss.block-components("checkout-payment"); | |||
@include scss.block-properties("checkout-payment"); | |||
|
|||
&-info { | |||
@include scss.block-components("checkout-payment-info"); | |||
@include scss.block-properties("checkout-payment-info"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
} | ||
|
||
&-divider-line { | ||
@include scss.block-components("checkout-payment-info-divider-line"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
|
||
&-divider-line { | ||
@include scss.block-components("checkout-payment-info-divider-line"); | ||
@include scss.block-properties("checkout-payment-info-divider-line"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
@include scss.block-properties("checkout-payment-info-divider-line"); | ||
} | ||
} | ||
|
||
&-form { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
@include scss.block-properties("checkout-payment-info-divider-line"); | ||
} | ||
} | ||
|
||
&-form { | ||
@include scss.block-components("checkout-payment-form"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
@include scss.block-properties("checkout-payment-info-divider-line"); | ||
} | ||
} | ||
|
||
&-form { | ||
@include scss.block-components("checkout-payment-form"); | ||
@include scss.block-properties("checkout-payment-form"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected browser feature "css-nesting" is not supported by Firefox 115, Chrome 109, Safari 15.6, Safari on iOS 15.6-15.7,16.1,16.3, Opera Mobile 73, UC Browser for Android 15.5, Samsung Internet 22,23, QQ Browser 13.1, KaiOS Browser 2.5,3.0-3.1 and only partially supported by Edge 117,118,119, Chrome 114,116,117,118,119, Safari 16.6,17.0,17.1, Opera 102,103,104, Safari on iOS 16.6-16.7,17.0,17.1, Android Browser 119, Chrome for Android 119 (plugin/no-unsupported-browser-features) plugin/no-unsupported-browser-features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a few change suggestions and there's one question I have on a bit of code. Otherwise it looks good. You can ignore the comment I made about fixing the translation string and blank line, looks like it was fixed while I was reviewing it.
import useSales from "./useSales"; | ||
|
||
const STRIPE_PAYMENT_METHOD_ID = 18; | ||
const PAYPAL_METOD_ID = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const PAYPAL_METOD_ID = 10; | |
const PAYPAL_METHOD_ID = 10; |
STRIPE_PAYMENT_METHOD_ID, | ||
stripeIntentsDefaultID, | ||
); | ||
const paypal = getPaymentMethodByID(options, PAYPAL_METOD_ID, paypalDefaultID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const paypal = getPaymentMethodByID(options, PAYPAL_METOD_ID, paypalDefaultID); | |
const paypal = getPaymentMethodByID(options, PAYPAL_METHOD_ID, paypalDefaultID); |
const items = currentOrder.items.map((item) => { | ||
const { sku, priceCode, quantity } = item; | ||
return { sku, priceCode, quantity }; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this being done just to filter out the other properties that can be on item
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is being done to filter out keys that are not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@edwardcho1231 , after merging in the latest from 2.1.2 I noticed an issue. Could you add unit test files for the new components you've added? The coverage threshold is pretty low right now as we're still working on fixing our tests for the new node version, so some basic unit tests should be enough for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edwardcho1231 , I can pass this for now, but could you and @mattkim93 make sure that these new changes to subs have proper test coverage. We've lowered requirements temporarily so things could pass, but it seems that there are still issues with the tests in the subs blocks.
11af306
into
arc-themes-release-version-2.1.2
* THEMES-1520: Enable testing (and linting THEMES-1460) (#1809) * THEMES-1520: Enable testing (and linting THEMES-1460) * Re-enable test on github action * try to get stylelint to ignore jsx * Attempt #2 to get stylelint to ignore jsx * ASUB-7844 PayPal block (#1806) * move stripe logic into a button * add paypal logic * clean up and try adding styles * clean up logic and create hooks * update styles for paypal * address feedback * add more strings for intl * address feedback * address feedback * add test for PayPal block --------- Co-authored-by: Vito Galatro <[email protected]> * ASUB-7844 Fix issue where identity is not defined in paymentInfo (#1831) fix issue where identity is not defined in paymentInfo * Fix paywall Issue (#1824) * fixing ArcP.run * attending feedback * attending feedback * attending feedback * attending feedback * attending feedback * ASUB-8002 Update payment method (#1813) * changes * payments * remove env file * update payment method - stripe Intents * update payment method * fixing typo * solving conflict --------- Co-authored-by: Vito Galatro <[email protected]> * ASUB-7844 Bug fix (#1836) fix issue with redirect when user is not logged in and adjust setTimeout delay * THEMES-1589: Subs phrases added for all locales (#1837) THEMES-1589: properly added new subs phrases. * Improvement update payment method - missing message (#1839) improvement update payment method- missing message * THEMES-1600: fixed more merge conflicts. * THEMES-1600: yet more merge conflicts. --------- Co-authored-by: nschubach <[email protected]> Co-authored-by: edwardcho1231 <[email protected]> Co-authored-by: LauraPinilla <[email protected]>
If you have not filled out the checklist below, the pr is not ready for review.
Description
Information about what you changed for this PR
Jira Ticket
Acceptance Criteria
copy from ticket
Test Steps
git checkout {branch name}
npx fusion start -f -l {blocks to link}
Effect Of Changes
Before
Example: When I clicked the search button, the button turned red.
[include screenshot or gif or link to video, storybook would be awesome]
After
Example: When I clicked the search button, the button turned green.
[include screenshot or gif or link to video, storybook would be awesome]
Dependencies or Side Effects
Examples of dependencies or side effects are:
Author Checklist
The author of the PR should fill out the following sections to ensure this PR is ready for review.
npm run lint
to check for errors. Often,npm run lint:fix
will fix those errors and warnings.npm run test:coverage
to see your progress.npm run test
, made sure all tests are passingplease explain why (so that we can fix it whenever it gets refactored).
Reviewer Checklist
The reviewer of the PR should copy-paste this template into the review comments on review.