Implement loading state to WooPay express checkout button #7366
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +2.24 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
@stephendharmadi please take a look at the demo video and LMK if you have any suggestions. I took advantage of the available WC blocks spinner class – also available in the email input loading state: email-input-loading-spinner.movThe color of the spinner is the same as the font color of the button theme. When it's disabled, it's styled with a |
| useEffect( () => { | ||
| const handlePageShow = ( event ) => { | ||
| // Re-enable the button after navigating back/forward to the page if bfcache is used. | ||
| if ( event?.persisted ) { | ||
| setIsLoading( false ); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener( 'pageshow', handlePageShow ); | ||
|
|
||
| return () => { | ||
| window.removeEventListener( 'pageshow', handlePageShow ); | ||
| }; | ||
| }, [] ); |
There was a problem hiding this comment.
Without this, and if back-forward cache is enabled in chrome://flags. The button is kept in its loading state after clicking the browser's back button.
Similar issue: #5113
ricardo
requested review from
a team and
rafaelzaleski
and removed request for
a team
|
Also cc'ing @nikkivias in case Stephen doesn't see this. We need a design feedback around midday Thursday, Oct 5th, to be able to merge this before WooPayments code freeze. Apologies for the short notice. |
| } ); | ||
| } | ||
| }; | ||
| } ); | ||
|
|
||
| return iframe; | ||
| }, [ isProductPage, context, isPreview, listenForCartChanges ] ); | ||
| }, [ isProductPage, isPreview, isLoading, context, listenForCartChanges ] ); |
There was a problem hiding this comment.
@ricardo, I just wanted to jump in here and highlight that by adding isLoading here, we inadvertently introduce a regression. For example, once we click the WooPay button, the isLoading state is updated to true, which in turn calls the const newIframe = useCallback(), which ultimately calls the useEffect() that appends the iFrame to the UI. This means we end up with duplicate iFrames in the UI. Once the isLoading state returns to false (for any given reason), we add another iFrame to the UI.
I suggest maintaining the isLoadingRef alongside isLoading and removing the isLoading dependency from const newIframe = useCallback(). Although not the ideal solution, this will prevent a newIframe from being created and added to the UI each time isLoading state updates. We may also want to add a note about this potential regression next to the dependencies.
There was a problem hiding this comment.
Thanks for pointing this out. I didn't notice the duplicate iframe when testing.
Indeed it doesn't seem ideal to keep both the isLoading state and isLoadingRef, but I also don't have a better suggestion.
Fixed in 6b351fc.
@lovo-h Not suggesting any changes, but just wondering, could the iframe be rendered as a React component inside the button perhaps? To avoid these non-react DOM manipulations.
There was a problem hiding this comment.
@lovo-h Not suggesting any changes, but just wondering, could the iframe be rendered as a React component inside the button perhaps? To avoid these non-react DOM manipulations.
In other places, we compose the iframe like we do here. But I think that if we render the iframe like a React component, that would address these issues. Good call.
|
Hi @ricardo, thanks for reaching out. Could you follow these button specifications? It covers the loading and disabled state of the WooPay express checkout button for different button styles (Dark, Light, Light with Outline).
|
It looks good! @ricardo |
|
@stephendharmadi the Figma you shared appears to be out-of-date with the screenshots attached in your #7366 (comment) (dark colors are different). I had assumed we would merge both "Disabled" and "Loading" styles, but now if I understand correctly, the loading state should not have lighter colors as in the disabled style. Right? So, here's what the updated demo looks like: demo-2.movThat way we simply keep the same colors as the "Resting" button, but technically the button is disabled while in its "Loading" state (It's not clickable, doesn't change when hovered, and has a I'll wait for your confirmation on the above by Friday. |
The artboard mode was accidentally switched. You can check again.
Yes, the color will be the same as the default state. |
Fixes #7295
Changes proposed in this Pull Request
Demo
demo.mov
Theme preview
Dark
dark.mov
Light
light.mov
Outline
outline.mov
Testing instructions
_wcpay_feature_woopay_first_party_auth. There should be no loading indicator without it.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge