-
Notifications
You must be signed in to change notification settings - Fork 269
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
[Website] Restore the single-click "Edit Settings" flow #1854
Conversation
Let's get it in! The last E2E run worked except of a single failure which I've addressed in the last commit. Happy to backtrack and hold on with deploying if a feedback comes surfacing a flaw here. |
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.
This looks great.
@@ -43,7 +43,7 @@ | |||
</script> | |||
</head> | |||
<body> | |||
<main id="root"> | |||
<main id="root" aria-label="WordPress Playground"> |
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 love how addressing elements of a page for E2E tests has overlapping concerns with A11Y (or maybe it is better said the other way around A11Y -> E2E-testability)
}); | ||
|
||
SupportedPHPVersions.forEach(async (version) => { | ||
/** | ||
* WordPress 6.6 dropped support for PHP 7.0 and 7.1 and won't load on these versions. | ||
* Therefore, we're skipping the test for these versions. |
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 would be good for our WordPress builds to encode minimum supported PHP versions and at least warn about incompatible version in UI. It looks like the WordPress.org API does list the minimum supported PHP version for each release. From
https://api.wordpress.org/core/version-check/1.7/?channel=beta :
{
"response": "development",
"download": "https:\/\/downloads.wordpress.org\/release\/wordpress-6.7-beta1.zip",
"locale": "en_US",
"packages": {
"full": "https:\/\/downloads.wordpress.org\/release\/wordpress-6.7-beta1.zip",
"no_content": false,
"new_bundled": false,
"partial": false,
"rollback": false
},
"current": "6.7-beta1",
"version": "6.7-beta1",
"php_version": "7.2.24",
"mysql_version": "5.5.5",
"new_bundled": "6.4",
"partial_version": false
}
If we had that info, we could stop hardcoding PHP versions here.
Created an issue for adding the info:
#1857
const siteManagerHeading = this.page.getByText('Your Playgrounds'); | ||
if (!(await siteManagerHeading.isVisible({ timeout: 5000 }))) { | ||
const siteManagerHeading = this.page.locator('.main-sidebar'); | ||
if (await siteManagerHeading.isHidden({ timeout: 5000 })) { |
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.
Nice. :) I didn't know about isHidden().
Motivation for the change, related issues
Restores the easily accessible "Edit settings" button Playground had before merging #1731, and refocuses the user experience on a single, temporary Playground. Multiple Playgrounds are still possible, but now they're less emphasized.
As we've learned from @annezazu and other users, the recent Multiple Playgrounds UI update made rapid fire iterations in Playground more difficult. Before #1731, we've had an easily accessible button to update WP and PHP versions. After #1731, that feature required multiple clicks and finding the right button.
This PR introduces the following changes:
Kudos to @jarekmorawski for thinking through and designing multiple variations of the user flows ❤️
Technical details
The implementation is straightforward and focused on rearranging React components and CSS. There's one gotcha in the process of saving temporary site settings. The settings form submission calls
window.history.pushState()
and theEnsurePlaygroundIsSelected
component watches for the URL changes. However, the user may click the "Update Settings & Reset Playground" button even without changing any form value. Normally, this would mean we can't detect a change and reset the Playground. This PR, thus, adds the?random=<random string>
parameter to the query string to allow Playground notice the change and recreate the temporary site.Visuals
Here's the video walkthrough – note I've recorded it before this PR was fully ready for a review:
CleanShot.2024-10-07.at.11.27.12.mp4
Follow up work
There are more design elements to consider, e.g. Snackbar notices. @jarekmorawski already designed some improvements and is working more. I would still like to get this PR in and continue iterating based on the feedback we get.
UI updates checklist
Testing plan
CI aside, interact with the design proposed in this PR and confirm it feels right.