Skip to content
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

fix(ia): tweak syndication plugin cards behavior #3707

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Jan 28, 2025

All Submissions:

Changes proposed in this Pull Request:

Tweaks the WizardsPluginCard component slightly:

  • Allows for a reloadOnActivation prop to be passed. If set to false, the card will not trigger a page reload after installing or activating its plugin. Deactivating the plugin will still result in a page reload, to clear out dead WP admin menu links. Default: true so that component instances not specifically touched by this PR are not affected.
  • If passed an editLink prop, will continue to show the "Configure" link to the plugin's settings page even after the plugin has been fully setup.
  • If the plugin isn't yet installed, calls the Plugin_Manager::activate() method instead of the install() method, as activate() will automatically install the plugin first if not installed. This lets the user skip an extra click and REST API roundtrip to install then activate the plugin.

Then, applies these changes to instances of the WizardsPluginCard component in the Syndication wizard page:

  • Sets both Apple News and Distributor cards to reloadOnActivation: false—these don't really require a refresh immediately upon activation as the "Configure" links that appear will work without a page reload.
  • Updates the plugin title and description for Publish to Apple News to exactly match those from the plugin itself.
  • Adds an editLink prop for the Distributor plugin which leads to the "External Connections" settings page.

How to test the changes in this Pull Request:

  1. Start with the Apple News and Distributor plugins not installed on your testing site.
  2. Navigate to Newspack > Settings > Syndication.
  3. Click the toggle button on the left-hand side of each plugin card. Confirm that upon success, both plugins are in a fully-activated state and show their "Configure" buttons, and that the "Configure" buttons lead to the correct plugin settings page.
  4. Delete both plugins and repeat step 3, but this time use the "Install " button on the right-hand side of the card. Confirm that the resulting behavior is the same as using the toggle button.
  5. Toggle off both plugins using the toggle button and confirm that the page reloads after toggling off each one. Also confirm that the plugins are actually set to a deactivated status.
  6. Smoke test the plugin cards elsewhere throughout the wizard pages and confirm that they still work as intended. Most exist in the following Settings subpages:
  • Connections
  • Social
  • Emails

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo dkoo added the [Status] Needs Review The issue or pull request needs to be reviewed label Jan 28, 2025
@dkoo dkoo self-assigned this Jan 28, 2025
@dkoo dkoo requested a review from a team as a code owner January 28, 2025 23:16
@miguelpeixe miguelpeixe self-requested a review January 29, 2025 12:36
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great changes and it's working well.

An issue I'm having on the epic branch and here, not introduced by these changes, is a weird inability to deactivate Apple News.

I followed the instructions to delete the plugin and install/activate via the card, but it is still unable to deactivate afterward.

apple-news-deactivate.mov

I'll investigate this further and we can tackle it in a separate PR.

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Jan 29, 2025
@miguelpeixe
Copy link
Member

miguelpeixe commented Jan 29, 2025

I think I got it. It's this line:

! statuses.isSetup ? onActivate() : onDeactivate();

I don't have the plugin configured (pluginState.configured), just activated.

This line needs to be ! statuses.isActive ? onActivate() : onDeactivate();

You can test it by removing the newspack_has_configured_plugin_publish-to-apple-news option and trying to deactivate the plugin.

Another weird UI glitch I just found is when I click to Configure for the first time it enters the loading state with the card toggled off before redirecting.

@dkoo
Copy link
Contributor Author

dkoo commented Jan 29, 2025

@miguelpeixe it seems like updating that line fixes both issues you mentioned—does it for you?

@miguelpeixe
Copy link
Member

Yep :shipit:

@dkoo dkoo merged commit 0cb39ff into epic/ia Jan 29, 2025
8 checks passed
@dkoo dkoo deleted the fix/syndication-plugin-cards branch January 29, 2025 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants