-
Notifications
You must be signed in to change notification settings - Fork 843
Revert NL-243 changes - implementation belongs in Calypso #46284
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
base: trunk
Are you sure you want to change the base?
Conversation
- Created ImportSubscribersModal component with CSV upload functionality - Added plan detection logic to show different messages for free vs paid plans - Free plan: Shows 100 subscriber limit with upgrade link - Paid plan: Shows 10,000 subscriber manual review message - Updated admin interface with "Add Subscribers" button - Added proper styling for modal and page layout - Updated tests to match new component structure Co-authored-by: robertbpugh <[email protected]>
- Fixed missing sprintf import in import-subscribers-modal.tsx - Cleaned up unused imports in admin.tsx - Added changelog entry for the feature Co-authored-by: robertbpugh <[email protected]>
- Remove .tsx extension from import statement per TypeScript best practices - Remove console.log statement and improve TODO comment - Enhance plan detection logic with explicit allowlist of free plan slugs Co-authored-by: robertbpugh <[email protected]>
|
@copilot Why did you "Created ImportSubscribersModal component" - Wouldn't you have edited the existing modal? |
I created a new component because there was no existing import modal in the subscribers-dashboard package. Before my changes, the package only had a "Hello world" placeholder in admin.jsx. If there's an existing modal that should be edited instead (perhaps in Calypso or another location), could you point me to it? The issue mentioned "the modal accessible from wordpress.com/subscribers" which suggests it might be in a different codebase. I couldn't find any existing subscriber import modal in the Jetpack repository to edit. |
|
@copilot This doesn't seem right. Can you check Calypso for the modal? |
You're correct. After investigating, I found that the "Import subscribers" feature links to The changes for NL-243 should be made in the Calypso repository instead. The import modal accessible from wordpress.com/subscribers is a Calypso component. Should I revert all changes from this PR since they were implemented in the wrong codebase? |
|
@copilot Yes, revert everything and implement in Calypso. |
Reverting all Jetpack changes as the subscriber import modal exists in Calypso, not in the Jetpack repository. The changes for NL-243 should be implemented in the Calypso codebase instead. Co-authored-by: robertbpugh <[email protected]>
All changes reverted in commit 09199cb. The subscribers-dashboard package is back to its original state with just the "Hello world" placeholder. The implementation for NL-243 should be done in the Calypso repository where the actual import modal exists. |
Proposed changes:
This PR reverts all changes that were initially made to implement NL-243 in the Jetpack repository. After investigation, it was determined that the subscriber import modal exists in Calypso (wordpress.com/subscribers), not in the Jetpack codebase.
Reverted changes:
ImportSubscribersModalcomponent and related filesadmin.jsxto original "Hello world" placeholder statepackage.jsonReason for revert:
The import modal accessible from wordpress.com/subscribers/#add-subscribers is a Calypso component. The implementation for NL-243 should be done in the Calypso repository instead.
Files reverted to original state:
_inc/admin.jsx- restored_inc/style.module.scss- restored_inc/test/admin.tsx- restoredpackage.json- restoredwebpack.config.js- restoredFiles removed:
_inc/admin.tsx_inc/components/import-subscribers-modal.tsx_inc/components/import-subscribers-modal.scsschangelog/add-nl-243-import-limit-contextThis PR can be closed as the work should be done in Calypso instead.
Other information:
Jetpack product discussion
https://linear.app/a8c/issue/NL-243
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Verify that the subscribers-dashboard package has been restored to its original state:
projects/packages/subscribers-dashboard/_inc/admin.jsxcontains only the "Hello world" placeholderimport-subscribers-modalfiles exist in the_inc/components/directorypackage.jsondependencies match the original state (no @wordpress/components or @automattic/jetpack-script-data added)💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.