-
Notifications
You must be signed in to change notification settings - Fork 51
Clean up siteForm from legacy #2147
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
Conversation
sejas
left a comment
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.
Yay! great catch. Thanks for cleaning the code.
I tested it by importing a new site.
importing-site.mp4
Let's also fix unit tests before merging.
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.
Pull request overview
This PR cleans up the site creation form by removing legacy code related to the import process and renaming SiteForm to CreateSiteForm for better clarity. The cleanup removes unused properties and interfaces that were previously associated with importing backups during site creation.
Key changes:
- Renamed
SiteFormcomponent toCreateSiteFormto better reflect its specific purpose - Removed legacy import-related functionality (
FormImportComponentand associated props) - Cleaned up unused imports and properties (
trashicon,useRef,ACCEPTED_IMPORT_FILE_TYPES,isDisabledparameter)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/modules/add-site/components/create-site.tsx | Updated import and usage to reference CreateSiteForm instead of SiteForm |
| src/modules/add-site/components/create-site-form.tsx | Renamed component from SiteForm to CreateSiteForm, removed legacy import functionality and unused props, cleaned up unused imports |
Comments suppressed due to low confidence (2)
src/modules/add-site/components/create-site-form.tsx:37
- The interface name
SiteFormPropsshould be renamed toCreateSiteFormPropsto match the component nameCreateSiteForm. This follows the codebase convention where component names match their Props interface names (e.g.,BlueprintIssuesModalusesBlueprintIssuesModalProps,ButtonusesButtonProps).
src/modules/add-site/components/create-site-form.tsx:61 - [nitpick] The interface name
SiteFormErrorPropsand the componentSiteFormErrorshould be renamed to match the new component name (e.g.,CreateSiteFormErrorPropsandCreateSiteFormError) for consistency, since the parent component was renamed fromSiteFormtoCreateSiteForm.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| className?: string; | ||
| } | ||
|
|
||
| interface SiteFormProps { |
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.
Copilot made a great suggestions. We can rename the props name to match the renamed component.
| interface SiteFormProps { | |
| interface CreateSiteFormProps { |
|
This is just a suggestion, and it does increase the scope of this refactor, but this component deserves a refactor to reduce the props drilling even further, IMO. The ideal scenario would be if |
📊 Performance Test ResultsComparing ebd9fe4 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
Great idea, but let's do it later as a separate PR |
Proposed Changes
While working on Add wpcom to onboarding I noticed:
So I decided to clean it up.
I was also thinking about moving
create-site-form.tsxinsidemodules/add-site-form, and also merging it withmodules/add-site/components/create-site, but to keep this PR easy for review, I decided that it's better not to do it right now.I was also thinking about merging create-site and create-site-form, but to make this PR easier for review, I decided not to do it. It's not something very critical, but more important to make the PR diff clearer and easier for review.
Testing Instructions