-
Notifications
You must be signed in to change notification settings - Fork 22
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 tags support to Constant Contact #1054
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.
Works well as described. I have some questions that may or may not be relevant.
includes/service-providers/constant_contact/class-newspack-newsletters-constant-contact-sdk.php
Outdated
Show resolved
Hide resolved
includes/service-providers/constant_contact/class-newspack-newsletters-constant-contact-sdk.php
Show resolved
Hide resolved
includes/service-providers/constant_contact/class-newspack-newsletters-constant-contact.php
Show resolved
Hide resolved
We are holding this until we get a resolution whether we are shipping this feature or not. See pamTN9-5Em-p2#comment-7332 |
Thanks for the comments and update, @leogermani—I was about to approve this one but will hold off until we ship the feature again. |
We can get a new review on this one now @dkoo , thanks! |
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 still working as described, but something I noticed that seems true of all ESPs: updates to the local list names in WP don't persist to the corresponding ESP tag/group. Is this known/intentional? To test:
- Create a local list, observe that it's created in the ESP
- Update the name of the local list in WP, observe that the name no longer matches in the ESP
- Subscribing to the local list as a reader still subscribes to the correct tag/group in the ESP, so it's more of a semantic/UX issue for publishers than a technical issue.
Another nitpick: when editing a single local list, the Newsletter > Lists menu item isn't expanded (although "Lists" is highlighted when you hover over "Newsletters").
At some point this was intentional, but we discussed that we should change this behavior. There's a task for that already:
Yeah I noticed that. I'll try to see if there's a workaround for it. |
Fixed the menu behavior in #1092 |
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.
Thanks for the explanation! Since we're handling those two issues in other tasks/PRs, this one looks good 👍
🎉 This PR is included in version 1.60.0-alpha.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.60.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
All Submissions:
Changes proposed in this Pull Request:
Adds support to Tags for the Constant Contact provider. This will enable the new Lists feature for this provider.
How to test the changes in this Pull Request:
Also note that the ability to select this segment in the editor when sending newsletters will be added in another PR (UPDATE: It was added in #1055)
Other information: