fix: add Link and WooPay compatibility tooltip #5515
Conversation
frosso
requested review from
a team and
timur27
and removed request for
a team
|
Size Change: +60 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
| <VisuallyHidden> | ||
| { __( | ||
| 'Link by Stripe cannot be enabled at checkout. Click to expand.', | ||
| 'woocommerce-payments' | ||
| ) } | ||
| </VisuallyHidden> |
There was a problem hiding this comment.
What's the purpose of these visually hidden components for both WooPay and Stripe Link?
There was a problem hiding this comment.
I added it so that screen readers can identify what the purpose of the tooltip.
The tooltip renders a button element around the SVG icon. The SVG icon is completely removed from the screen reader's text. So, without the VisuallyHidden element, a screen reader user would only "see" a button element with no text.
Adding a VisuallyHidden element adds some text for SR users, so that they can better understand the purpose of the button element.
An alternative could also have been to add an aria-label attribute on the button element.
I noticed that this article gives a decent overview of the problem: https://www.sarasoueidan.com/blog/accessible-icon-buttons/
| label={ __( | ||
| 'Link by Stripe', | ||
| 'woocommerce-payments' | ||
| ) } |
There was a problem hiding this comment.
I guess the label prop has been added to be more 'correct' technically when using this component from Blocks? I can see that we add it and hide it with the display: none, so I'm curious what was the reasoning behind it.
There was a problem hiding this comment.
I believe it has been added so that it can be more easily targeted within the tests. I noticed that some tests used it, others didn't.
| @@ -50,11 +50,11 @@ describe( 'ExpressCheckout', () => { | |||
| const updateIsPaymentRequestEnabledHandler = jest.fn(); | |||
|
|
|||
| useGetAvailablePaymentMethodIds.mockReturnValue( [ 'link', 'card' ] ); | |||
| useEnabledPaymentMethodIds.mockReturnValue( [ [ 'card', 'link' ] ] ); | |||
| useEnabledPaymentMethodIds.mockReturnValue( [ [ 'card' ] ] ); | |||
There was a problem hiding this comment.
I had to tweak the tests a bit: before, both WooPay and Link were enabled.
With these changes, this test shows only one of them enabled at the same time.
| @@ -172,4 +163,35 @@ describe( 'ExpressCheckout', () => { | |||
| const linkCheckbox = container.getByLabelText( 'Link by Stripe' ); | |||
| expect( linkCheckbox ).not.toBeChecked(); | |||
| } ); | |||
|
|
|||
| it( 'should prevent enabling both Link and WooPay at the same time', async () => { | |||
There was a problem hiding this comment.
Added a test for the added behavior.
| @@ -142,9 +177,13 @@ const ExpressCheckout = () => { | |||
| </div> | |||
| </li> | |||
| ) } | |||
| <li className="express-checkout has-icon-border"> | |||
There was a problem hiding this comment.
The previous changes had a has-icon-border classname wrapper. But it is not needed on this component. The SVGs already have a border.
| <div className="express-checkout__checkbox"> | ||
| <CheckboxControl | ||
| label={ __( |
There was a problem hiding this comment.
Added a label here, just for consistency
| label={ __( | ||
| 'Link by Stripe', | ||
| 'woocommerce-payments' | ||
| ) } |
There was a problem hiding this comment.
I believe it has been added so that it can be more easily targeted within the tests. I noticed that some tests used it, others didn't.
| <VisuallyHidden> | ||
| { __( | ||
| 'Link by Stripe cannot be enabled at checkout. Click to expand.', | ||
| 'woocommerce-payments' | ||
| ) } | ||
| </VisuallyHidden> |
There was a problem hiding this comment.
I added it so that screen readers can identify what the purpose of the tooltip.
The tooltip renders a button element around the SVG icon. The SVG icon is completely removed from the screen reader's text. So, without the VisuallyHidden element, a screen reader user would only "see" a button element with no text.
Adding a VisuallyHidden element adds some text for SR users, so that they can better understand the purpose of the button element.
An alternative could also have been to add an aria-label attribute on the button element.
I noticed that this article gives a decent overview of the problem: https://www.sarasoueidan.com/blog/accessible-icon-buttons/
Changes proposed in this Pull Request
Adding a tooltip that is displayed whenever the merchant attempts to enable both WooPay and Link at the same time.
I didn't run the changelog command because it's not needed - this is getting merged into a larger feature branch
Fixes #4720
Testing instructions
I recommend reviewing these changes with the "Hide whitespace changes" setting enabled - it'll show you that it's much fewer changes: https://github.com/Automattic/woocommerce-payments/pull/5515/files?w=1
With this changes, it's still possible that a merchant has both Link and WooPay enabled at the same time. But the number of merchants that had WooPay enabled is so small, that it doesn't matter.
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