Site editor template preview: add E2E test and aria-pressed attribute to template preview toggle #56096
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It also adds an aria label to the edit template button to indicate its state (pressed or not pressed).
ramonjd
added
[Type] Code Quality
ramonjd
commented
Nov 14, 2023
| } ) | ||
| .click(); | ||
|
|
||
| // Add some content to the page. |
There was a problem hiding this comment.
This is all pretty verbose and declarative. I don't see much point in abstracting since it's unique to the test.
|
Size Change: +54 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
andrewserong
approved these changes
Nov 14, 2023
There was a problem hiding this comment.
Thanks for adding in the extra e2e test to cover this!
✅ Pressed state appears to be working correctly with no visual change, but now the aria-pressed state is flagged for the toggle button
✅ Test coverage looks good and covers the main behaviour of the template preview toggle
Just left a comment about a potential line removal that could speed up the test slightly. This part of the site editor is quite slow to test, so another option for a follow-up could be to try to consolidate this test with one of the other tests in this file, so that we don't have to wait for a full page load. The downside of doing that would be that it could negatively affect readability of the tests, but the potential upside could be to speed up tests by reducing the number of full page loads that we wait for. In any case, not a blocker for now!
LGTM! ✨
| await page.keyboard.type( 'Sweet paragraph 1' ); | ||
| await page.keyboard.press( 'Enter' ); | ||
| await page.keyboard.type( 'Sweet paragraph 2' ); | ||
| await editor.saveSiteEditorEntities(); |
There was a problem hiding this comment.
Do we need to save the site editor entities in order to test this? While running locally, it looks like we might be able to save around half a second in runtime of the test if we skip saving the entities here.
There was a problem hiding this comment.
Thanks for checking!
No, I guess we don't need to save. I think it stemmed from my data-loss anxiety. 😄
Thanks for testing and for the thoughts! The spec is a bit of a beast as it is. I can take another pass in a follow up PR to see if we can consolidate as you say - sounds like it's worth it. Any subsequent tests can, I guess, reload as necessary. |
|
Thanks for this addition. |
Just a heads-up that you can simply run |
Awesome, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Add an E2E test for the changes in
The E2E test intends to cover the following while editing a page in the site editor:
Also, we're adding an
aria-pressedattribute to the "Toggle preview button" to indicate it's state. Currently there's only a visual indicator in the form of an SVG check.Why?
Coz there isn't a test.
Testing Instructions
Run the tests if you like! Or see that they are passing on CI.
A great way to run playwright E2E tests individually or as suites is using the UI tool:
npx playwright test --ui --headed --config test/e2e/playwright.config.tsTo check the ARIA attribute:
aria-pressedattribute toggles accordingly.