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

Publicize: connections should always be an array #17436

Closed
wants to merge 1 commit into from

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Oct 9, 2020

Changes proposed in this Pull Request:

See #17420 (review)

connections really should be an empty array when there are no connections, rather than undefined.

Jetpack product discussion

  • N/A

Does this pull request change what data or activity we track or use?

  • N/A

Testing instructions:

  • Go to Pages > Add New
  • See no editor errors
  • Go to Posts > Add New
  • See Publicize Twitter options

Proposed changelog entry for your changes:

  • N/A

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Publicize Now Jetpack Social, auto-sharing [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Pri] Normal labels Oct 9, 2020
@jeherve jeherve added this to the 9.1 milestone Oct 9, 2020
@jeherve jeherve requested a review from pento October 9, 2020 13:34
@jeherve jeherve self-assigned this Oct 9, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D50929-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@jetpackbot
Copy link

Scheduled Jetpack release: November 3, 2020.
Scheduled code freeze: October 27, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17436

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against 61b08de

@pento
Copy link
Contributor

pento commented Oct 12, 2020

I investigated this some more, and found a few things:

First of all, the fix in #17420 is the most correct option for Jetpack. PublicizeTwitterOptions gets components from the post meta jetpack_publicize_connections value. Since this isn't registered for the page post type, it's reasonable that it could return undefined.

However, this doesn't address the broader question of why PublicizeTwitterOptions is even being rendered here. The answer to that can be found in a minor note of the PostTypeSupportCheck documentation:

If the post type is not yet known, it will be assumed to be supported.

It appears that the post type data is not available during the initial render, so PostTypeSupportCheck will briefly allow the Publicize components to be rendered, though they'll be removed again before they can be painted in the browser window. This was intended to be fixed (see: WordPress/gutenberg#7748), but it appears that it never was.

Unfortunately, fixing it is not as simple as changing the default return value for PostTypeSupportCheck. Since plugin sidebars use Slot Fills (which function in a FIFO manner), that would cause sidebar panels which don't have a PostTypeSupportCheck wrapper to be placed in the DOM before those that do.

At any rate, this PR can be closed, as it doesn't actually fix what I thought it fixed. 🙃

@jeherve jeherve closed this Oct 12, 2020
@kraftbj kraftbj removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Publicize Now Jetpack Social, auto-sharing [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants