-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
Remove title from checkout page #47529
Conversation
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
@@ -21,7 +21,7 @@ test.describe( 'Test the checkout template', () => { | |||
await expect( | |||
page | |||
.frameLocator( 'iframe[title="Editor canvas"i]' ) | |||
.locator( 'h1:has-text("Checkout")' ) | |||
.locator( 'div:has-text("Place Order · <price/>")' ) |
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've found it a good practice to let Playwright determine the selectors for me as it takes care of the recommended practices and the selector uniqueness on the page. It sometimes needs to be tweaked a little bit (e.g., we use editor.canvas.*
instead of explicit page.frameLocator( 'iframe[title="Editor canvas"i]' ).*
), but it does the job right most of the time.
I've recorded a little vid to demo how I use the Plawright codegen for that in the WC context, as it's easier than writing the steps! :D
Screen.Recording.2024-05-16.at.16.53.09.mov
To run the npx playwright open localhost:8889
command, you must be in the /woocommerce/plugins/woocommerce-blocks/
folder. Also, the local e2e test env must be running (pnpm env:start
).
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.
Thanks a lot for pointing this out, @WunderBart. I went ahead and replaced all page.frameLocator( 'iframe[title="Editor canvas"i]' ).*)
sections.
Hi @WunderBart, @wavvves, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
@WunderBart As I've improved a few e2e tests, based on your feedback, I would appreciate it if you could review the test-related changes. As the tests are passing, they should be fine. It's just to be on the save side. 😀 |
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.
It looks great from the test perspective! 🎉 Thank you for applying the editor.canvas.*
convention everywhere! 🙇
Thank you for reviewing the test-related changed, @WunderBart. |
@wavvves As Bart reviewed the test-related changes, you can focus on the functional changes. |
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.
Thank you for working on this Niels, it's testing fine for me, just note that you need to reset customization for this to work.
Thanks for reviewing and approving this PR, @senadir. I'll merge it for now, to get it in before the code freeze, and will update the testing instructions the next days. |
By removing the Page title we are impacting the accessibility of this page. Among other impacts, screen reader users won't have an indication of the page currently visited. @nielslange I think we should reconsider our approach from this PR. My recommendation would be to have a top lead H1 heading in the page, that we visually hide, but is still available for screen readers to parse. It's worth exploring if there are other options or what this approach implies. Could you create a follow up ticket for this? @PanosSynetos this will most likely need to be accommodated in your tests. @nikkivias & @pmcpinto for awarness |
Thanks @ralucaStan for the heads up - @nielslange could you please share a PR, before you merge it, so @woocommerce/somewherewarm can take care of the tests as early as possible? Thanks! |
@ralucaStan: I just created #48619.
@PanosSynetos: I'm not sure if I'll work on this issue, but I added a note on the issue that whoever works on the PR should contact you before merging the PR. |
Changes proposed in this Pull Request:
Remove the title from the Checkout template so that it does no longer appear on the checkout page in the frontend.
Closes #47509.
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
WP Admin » Apperances » Editor » Pages » Checkout
.Checkout
title.WP Admin » Apperances » Editor » Templates » WooCommerce » Page: Checkout
.Checkout
title.Checkout
title./wp-admin/edit.php?post_type=page
./wp-admin/admin.php?page=wc-settings&tab=advanced
.Checkout
title.Changelog entry
Significance
Type
Message
Remove title from checkout page
Comment