Studio: Add a warning when pushing a site with an outdated WordPress version #2136
Conversation
📊 Performance Test ResultsComparing c654224 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
|
|
||
| const latestWpVersion = wpVersions.find( ( version ) => version.value === 'latest' )?.value; | ||
| const latestWpVersion = wpVersions.find( | ||
| ( version ) => version.value !== 'latest' && ! version.isBeta && ! version.isDevelopment |
There was a problem hiding this comment.
The tooltip on the Create preview button was broken for the semver comparison of versions so I am fixing it at the same time. Alternative fix, could be to add another prop to interface WordPressVersion with the actual version but it seemed like a bit of extra for this one edge case with latest. (that's what I initially tried)
The reason why this is broken: We need value: 'latest' for the dropdown selector, but we need the actual version number like '6.7.1' for semver comparisons. Before this fix, the version mismatch detection was completely broken because it was trying to compare '6.2' with the string 'latest'.
|
One last thing is that we can potentially combine these two messages in one notice when both have to be displayed but I kept them separate for now as they are relevant for two different things: 
|
katinthehatsite
changed the title
There was a problem hiding this comment.
Nice! The changes work as expected. 👌🏼
One thing that came to my mind is how the scrolling would look like if we added a similar opacity as we have on the Sync tab:
I believe it could help users noticed the content is scrollable in cases like this one:
At the same time, this may be outside of scope for this PR.
One last thing is that we can potentially combine these two messages in one notice when both have to be displayed but I kept them separate for now as they are relevant for two different things:
I agree and think it makes sense to keep them separate. 👍🏼
I left one inline comment. Also noticed that unit tests are failing.
sejas
left a comment
sejas
left a comment
There was a problem hiding this comment.
Great work Kat! I saw in the Linear issue that the notice was preferred over the tooltip 👍 .
I only noticed that the Database checkbox appears a bit cut. Could we show it all?
| title={ syncTexts.title } | ||
| > | ||
| <div className={ isPushSelectionOverLimit ? 'pb-[140px]' : 'pb-[70px]' }> | ||
| <div className={ footerPadding }> |
There was a problem hiding this comment.
Not a strong opinion, but we could declare the footer padding inside the class. Something like:
<div
className={ cx(
noticeCount === 0 && 'pb-[70px]',
noticeCount === 1 && 'pb-[140px]',
noticeCount >= 2 && 'pb-[210px]'
) }
>
…ice-for-different-wordpress-versions
…ice-for-different-wordpress-versions
Nice, good catch! I was testing on bigger screen so it looked fully fitting there. I will adjust the padding a bit more 👍 Updated in e423be9 |
|
@ivan-ottinger I also added a background similar to the other footer for "Connect another site" . Feel free to give it another review 👍 |
ivan-ottinger
left a comment
ivan-ottinger
left a comment
There was a problem hiding this comment.
@ivan-ottinger I also added a background similar to the other footer for "Connect another site" . Feel free to give it another review 👍
Thank you for the updates, Kat.
The changes look good and the notice is rendering correctly. 👍🏼
…ice-for-different-wordpress-versions
Related issues
Closes STU-875
Proposed Changes
This PR adds a notice for the user when the PHP or WordPress versions on their Studio site are not the latest and are different from the versions used on WP.com in the Sync modal for the push process.
Testing Instructions
npm start6.4PushoptionPre-merge Checklist