-
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
Add migration instructions basic structure #91853
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~68 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~167 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. |
e3b0456
to
8b3ebc6
Compare
8b3ebc6
to
d48058e
Compare
d48058e
to
223c0b0
Compare
@@ -202,105 +202,98 @@ const Sidebar = ( { | |||
return ( | |||
<> | |||
{ site && <QueryMembershipsSettings siteId={ site.ID } source="launchpad" /> } | |||
<div className="launchpad__sidebar"> |
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.
The changes in this file only remove these wrapper div launchpad__sidebar
because it was moved to the LaunchpadContainer
component.
@@ -2,7 +2,7 @@ | |||
@import "@automattic/onboarding/styles/mixins"; | |||
|
|||
.import-hosted-site, | |||
.site-migration, | |||
.site-migration :not(.site-migration-instructions--launchpad), |
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 don't want to apply the old styles to the new migration instructions step.
ba73822
to
0c2abdc
Compare
max-width: 360px; | ||
} | ||
|
||
// TODO: Migrate to the checklist components. The checklist could receive a special className as prop that would be responsible for this. |
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.
For now, I kept it as it is, but it would be nice to change it at some point to be a responsibility of the checklist.
stepName="site-migration-instructions" | ||
isFullLayout | ||
hideFormattedHeader | ||
className="is-step-site-migration-instructions site-migration-instructions--launchpad" |
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.
site-migration-instructions--launchpad
is a class to differentiate it from the site-migration-instructions-i2
. I didn't want to give an i3
name to have a more semantic name in this case.
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.
Did you consider just creating a new step component with its own styles? This smells kinda fragile. :-) (Though I only had very cursory look)
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.
Hi @simison! Thank you for checking this!
new step component with its own styles?
Unfortunately, I think it wouldn't help with this case because the original class that we are trying to avoid is related to the flow itself.
We have some classes in client/landing/stepper/declarative-flow/internals/steps-repository/importer-migrate-message/style.scss
applied to the whole site migration flow (.site-migration
). And, among other things, one thing these classes are applying is a max-width: 720px;
to the step-container__content
, which we don't want in this step.
Considering this, let me know if you have any other suggestion to handle this. 😉
</div> | ||
</main> | ||
<LaunchpadContainer | ||
headerClassName="launchpad__sidebar-header" |
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.
In the future, if we could kill all of these classes and the overrides, keeping only the new ones from the StepContainer
, it would be ideal.
But I don't feel confident enough to do it without breaking things that I wouldn't think about.
@daledupreez and @simison, I added you from GitHub reviewer suggestions. If you have some time, I'd appreciate a quick check just to make sure I didn't miss anything with the refactor part of the current Launchpad step (under |
I will only be able to take a look tomorrow, @renatho, but hopefully someone else from Serenity can take a look before then. |
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.
Changes are mostly looking good and tested well. I just have a couple of questions.
👍 for the tests!
|
||
return ( | ||
<div className="site-migration-instructions__questions-wrapper"> | ||
{ translate( 'Questions?' ) }{ ' ' } |
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: Do we really need this space({ ' ' }
) here? If it's a spacing issue, we could add a margin to the link.
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.
Thank you for the suggestion! Applied here: 2bc43e1.
I also took the opportunity to move that to a different file.
<iframe | ||
className="migration-instructions-from-preview__iframe" | ||
title={ translate( 'Preview of the site being imported' ) } | ||
src={ fromUrl } | ||
/> |
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.
Should we have a fallback in case the site can't be loaded? Aren't there a few ways to prevent sites from being embedded?
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.
Yes! We need to make something here!
I had noticed the same, but since this PR was blocking other PRs I had created this issue to merge this one earlier.
Also, if you have any suggestion of a way it was solved somewhere already, it would be very helpful! :)
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!
Also, with the rounded borders, the scrollbar is being cut off when it's at the very top. Perhaps it would be best not to have rounded corners: Screen.Recording.2024-06-21.at.11.07.25.AM.mov |
f72c384
to
eab1ce7
Compare
eab1ce7
to
6827131
Compare
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 |
@renatho Is it possible to not force push? It's making review a bit difficult. 😅 |
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.
The issues I highlighted with the UI have been addressed. 👍🏻
Hey @donnapep! Sorry for that! My reason for that force pushes is not to pollute the commits with small tweaks I notice after pushing. But I only force push when it's not being reviewed and I never change commits already reviewed to avoid confusion. Is there a specific case where it got confused to see if I can improve it? |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/15329742 Some locales (Hebrew, Japanese) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Thank you @renatho for including a screenshot in the description! This is really helpful for our translators. |
There was a force push between my last review when I reported the bugs and your asking for another review. There's a section on force-pushing in the team handbook PdxWSz-61-p2 in case it's helpful. 🥂 |
Translation for this Pull Request has now been finished. |
Related to #91776
Proposed Changes
Known issues
Testing Instructions
migration-flow/new-migration-instructions-step
./setup/build/launchpad?siteSlug={SITE_SLUG}&siteId={SITE_ID}&showLaunchpad=true
Pre-merge Checklist