-
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
Nav Redesign: Add "Activate" ability to "Dev Tools" #91141
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~41 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~8209 bytes removed 📉 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~1061 bytes removed 📉 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. 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. |
|
||
setTimeout( () => { | ||
window.location.assign( to ); | ||
}, timeoutDuration ); |
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.
After the site Atomic transfer is complete, we still need a few moments for "things" to update. Redirecting right away does not work.
I tried to avoid this setTimeout
by leveraging requestSite( siteId )
and trying waiting on various "promises" and "states", but ultimately I was only able to get it working consistently with this timeout.
I set a conditional on timeoutDuration
so that we don't add unnecessary process time to the original flow. It only waits for our flow as it is.
I believe this method to be adequate, but welcome better alternatives!
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.
Could the WatiForAtomic support the feature
query string?
If it's provided, the step will poll the latest site data and do the redirection only when the feature is ready. What do you think?
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.
Done by 3886a34
client/landing/stepper/declarative-flow/transferring-hosted-site-flow.ts
Outdated
Show resolved
Hide resolved
<h1> { translate( 'Unlock all developer tools' ) }</h1> | ||
<h1> | ||
{ showHostingActivationButton | ||
? translate( 'Activate hosting configuration' ) |
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.
nit: Should it be Activate all developer tools?
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.
Can we align the modal design with @lucasmendes-design's design from https://github.com/Automattic/dotcom-forge/issues/7348#issuecomment-2125662159
I see, from reading the codebase, there could be multiple warnings. But I think at least we should rename / remove
=>
|
|
The |
I agree 😅 for some people it looks like the feature is broken. Currently it is also weird to still have a clickable close button in the modal while the button is still loading. |
After the flow finishes, the |
I recorded my test, not working because after the activation the URL gets the slug for simple rather than the site ID. Screen.Recording.2567-05-28.at.12.57.05.mov |
c18060f
to
a58ee7f
Compare
I'm passing the param |
Sounds good! |
The main issue was the stepper flow didn't sync the redux store to local storage so the site dashboard got the outdated data and then redirected you back to the dev tool. It should be addressed by adding the persistOnChange to the stepper flow There was another issue is that the |
c9072ee
to
75002b9
Compare
75002b9
to
06ec54f
Compare
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.
Tested well for me 🥳
Can we pass a param (&redirect=/wp-admin/options-general.php?page=options-permalink
) to redirect to the features from the upsell pages? That would make sense for the upsell flow from those pages. I'll go ahead and propose a PR later.
Screen.Recording.2567-05-28.at.20.02.55.mov
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.
LGTM!!! 🚀
Let's do it in a folllow-up PR 😄 |
I'll follow up with https://github.com/Automattic/dotcom-forge/issues/7492 to use params to open the activation modal and handle the redirection afterward. I'll investigate and propose a PR. @arthur791004, do you have any tips about where/how to handle those params? |
I think you can use the query params to determine the initial state of the Next, get the value of the |
Related to https://github.com/Automattic/dotcom-forge/issues/7348
Proposed Changes
The video below is cropped off center on purpose.
Screen.Recording.2024-05-24.at.5.43.45.PM.mp4
Why are these changes being made?
Testing Instructions
Regression test:
transferring-hosted-site-flow.ts
. Test this flow to ensure it still works.Pre-merge Checklist