-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Staging Sites: Add Staging Sites UI in Hosting Configuration #73709
Staging Sites: Add Staging Sites UI in Hosting Configuration #73709
Conversation
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.
@Automattic/yolo please review the PR.
I would also appreciate looking at rendering, as I think that component still renders too often when I unfocus/focus the browser. I suspect I'm still missing something there. Or maybe I'm trying to do premature optimization?
let transferInterval: NodeJS.Timeout; | ||
|
||
if ( ! isFetchingTransferStatus ) { | ||
transferInterval = setInterval( |
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.
Initially, I tried to do it using waitFor
like it's done in another component:
wp-calypso/client/my-sites/checkout/checkout-thank-you/marketplace/marketplace-thank-you.tsx
Line 118 in f8823ab
waitFor( 2 ).then( () => dispatch( fetchAutomatedTransferStatus( siteId ) ) ); |
Unfortunately, it was sending tens of requests to the server. Do you have any thoughts on what could cause it? Does waitFor
also need a way to cancel the effect?
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.
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 works great, I was able to create my staging site without errors. Love it! Great work! 🥳
I had to sandbox the API D102075-code
Some suggestions for future follow-ups/discussions related to the UI:
- We could set a minimum height to the
staging-site-card
so the card linking won't break the scrolling. - Both buttons
Create staging site
andManage staging site
are too similar, which can confuse the user. We could change the text, or use the secondary button style for manage the site, or add some icons. - Currently there is no easy way to "remove/refresh" the staging site.
|
||
return ( | ||
<Card className="staging-site-card"> | ||
<MaterialIcon icon="build" size={ 24 } /> |
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.
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.
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.
CC @javierarce
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.
We could also use Gutenberg icons: https://wordpress.github.io/gutenberg/?path=/story/icons-icon--library
We also have the Gridicons: https://automattic.github.io/gridicons/
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.
Going with the Share icon for now:
I created a follow up issue to explore further https://github.com/Automattic/dotcom-forge/issues/1856
Thanks @sejas , those are good suggestions. I've already added a ticket for the last one: 1757-gh-Automattic/dotcom-forge and added another one for card height 1840-gh-Automattic/dotcom-forge |
@javierarce any thoughts on this? Note that the "Manage staging site" button will be accompanied by "Delete staging site" button |
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 is pretty great, @wojtekn. Nice work!
}, | ||
onSuccess: () => { | ||
dispatch( recordTracksEvent( 'calypso_hosting_configuration_staging_site_add_success' ) ); | ||
dispatch( successNotice( __( 'Staging site was added.' ), noticeOptions ) ); |
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.
dispatch( successNotice( __( 'Staging site was added.' ), noticeOptions ) ); | |
dispatch( successNotice( __( 'Staging site added.' ), noticeOptions ) ); |
This notice disappeared really quickly for me (much quicker than 3 seconds). Maybe we should make it always present?
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.
Actually, now I see we don't need this notice. We display it when the simple site is added, but we still need to wait and poll for the transfer status. What if we remove it completely and solve it under 1767-gh-Automattic/dotcom-forge ?
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.
@wojtekn I think a notice would be nice. Can we have it displayed after the staging site is fully created?
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.
Moved with 6ef8d1e
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~693 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
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 work on this, @wojtekn !
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7833935 Thank you @wojtekn for including a screenshot in the description! This is really helpful for our translators. |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7833935 Thank you @wojtekn for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Related to 1696-gh-Automattic/dotcom-forge
Proposed Changes
In this PR, I propose to add an initial UI that allows adding a staging site.
Testing Instructions
Pre-merge Checklist