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

Likes and sharing panel: Render on demand #12052

Merged
merged 2 commits into from
Apr 15, 2019

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Apr 15, 2019

Likes and sharing are two independent plugins that render checkboxes in the same panel of the Jetpack sidebar.

There is an issue where the absence of other, unrelated sidebar panels causes the entire sidebar to be hidden, unintentionally hiding likes and sharing options:

if ( ! fills.length ) {
return null;
}
return (
<Fragment>
<PluginSidebarMoreMenuItem target="jetpack" icon={ <JetpackLogo /> }>
Jetpack
</PluginSidebarMoreMenuItem>
<PluginSidebar name="jetpack" title="Jetpack" icon={ <JetpackLogo /> }>
{ fills }
<JetpackLikesAndSharingPanel.Slot />
</PluginSidebar>
</Fragment>
);


The likes and sharing panel becomes another plugin, very similar to the Jetpack sidebar. This allows slot/fill to work correctly at all the levels. It is an independent plugin so that it can register (render) the slot 1 time while being used by 0 or more independent plugins to cause the panel to render 0 or 1 times as necessary.

The panel is also a true fill of the Jetpack sidebar, fixing the initial problem where it was a sibling of the fills. The sidebar is not rendered if there are no fills, which is why the sibling approach was a problem. As a true fill, the existing mechanism to show/hide the sidebar remains untouched.

This PR also includes some Janitorial work around the existing sidebar and the slot/fills:

  • Better use of named/default exports
  • Do not attach slot to the fill as FillComponent.Slot. These slots are not intended to be rendered outside of these components, do not export them.
  • Remove redundant component wrapping

Testing instructions (from #12045):

  1. Edit both posts and pages and ensure that the Jetpack sidebar is visible when necessary (sidebar contains panels).
  2. Enable and disable the Likes and Sharing button modules and ensure that checkboxes are only shown for active modules.
  3. Finally, ensure the checkboxes still toggle the buttons on the front-end correctly.

Supersedes:

@sirreal sirreal added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Sharing Post sharing, sharing buttons [Feature] Likes [Type] Janitorial [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Apr 15, 2019
@sirreal sirreal requested review from codebykat and a team April 15, 2019 12:11
@sirreal sirreal self-assigned this Apr 15, 2019
@sirreal sirreal marked this pull request as ready for review April 15, 2019 12:21
@sirreal sirreal requested a review from a team April 15, 2019 12:21
@sirreal sirreal force-pushed the update/likes-and-sharing-independent-sidbar branch from 2271fac to 63d3865 Compare April 15, 2019 12:22
@jetpackbot
Copy link
Collaborator

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 63d3865

@sirreal sirreal changed the title Likes and sharing panel: Decouple Likes and sharing panel: Render on demand Apr 15, 2019
@sirreal sirreal added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Apr 15, 2019
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Code looks good and this tests well and identically to #12045 (review)

Earlier behaviour wasn't hiding this only from pages, but also for non-public custom post types such as re-usable blocks where this probably shouldn't show up anyway. tThat's outside this refactoring and a separate bug and not stopping this from being merged.

Copy link
Member

@roccotripaldi roccotripaldi left a comment

Choose a reason for hiding this comment

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

Well done! A call to registerPlugin for the Likes and Sharing panel was what my prior implementation was missing. Thanks for fixing that!

@roccotripaldi
Copy link
Member

i'll apply manually to .com

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

this works well on my end 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 15, 2019
@sirreal
Copy link
Member Author

sirreal commented Apr 15, 2019

This should be good to merged, but I'm blocked by the wpcom check.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello sirreal! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D26942-code before merging this PR. Thank you!

@jeherve
Copy link
Member

jeherve commented Apr 15, 2019

@sirreal Should be good now. D26942-code is the matching WordPress.com diff.

@roccotripaldi roccotripaldi merged commit 12e8ed7 into master Apr 15, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 15, 2019
@roccotripaldi roccotripaldi deleted the update/likes-and-sharing-independent-sidbar branch April 15, 2019 17:53
@codebykat
Copy link
Member

THANK YOU @sirreal @roccotripaldi @simison @jeherve for moving so fast on this one 🎉

Best way to start a Monday!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Likes [Feature] Sharing Post sharing, sharing buttons [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended [Type] Janitorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants