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

Facebook widget: hide-cta and small-header #13744

Merged
merged 8 commits into from
Mar 17, 2020
Merged

Facebook widget: hide-cta and small-header #13744

merged 8 commits into from
Mar 17, 2020

Conversation

wigglemuff
Copy link
Contributor

@wigglemuff wigglemuff commented Oct 14, 2019

Changes proposed in this Pull Request:

Fixes #9270

Testing instructions:

  • Go to widgets page, add Facebook widget, add URL and save.
  • Go to front page, inspect the facebook widget, you will see data-hide-cta="false" data-small-header="false"
  • Go back to widget settings and enable both hide call to action and show small header options.
  • Now you will see data-hide-cta="true" data-small-header="true" in the element inspector.

Proposed changelog entry for your changes:

Screenshot:

Screen Shot on 2019-10-14 at 08:00:19

Other notes:

The third functionality adapt-container-width is covered by this #9889 issue and this #13740 PR.

@wigglemuff wigglemuff added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Pri] Normal Good For Community labels Oct 14, 2019
@wigglemuff wigglemuff requested a review from a team October 14, 2019 02:34
@wigglemuff wigglemuff self-assigned this Oct 14, 2019
@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link

jetpackbot commented Oct 14, 2019

Warnings
⚠️

pre-commit hook was skipped for one or more commits

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 54509bb

'stream' => '',
'cover' => 'true',
'hide_cta' => '',
'small_header' => '',
Copy link
Contributor Author

@wigglemuff wigglemuff Oct 14, 2019

Choose a reason for hiding this comment

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

Need to fix issues with spacing (in multiple places) to make it better formatted. Will do that after initial review by someone! :)

modules/widgets/facebook-likebox.php Outdated Show resolved Hide resolved
modules/widgets/facebook-likebox.php Outdated Show resolved Hide resolved
modules/widgets/facebook-likebox.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 14, 2019
add esc_html_e instead of _e

Co-Authored-By: Jeremy Herve <jeremy@jeremy.hu>
@matticbot
Copy link
Contributor

codestor4, Your synced wpcom patch D33973-code has been updated.

@stale
Copy link

stale bot commented Jan 24, 2020

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Jan 24, 2020
@stale stale bot removed the [Status] Stale label Feb 24, 2020
@matticbot
Copy link
Contributor

codestor4, Your synced wpcom patch D33973-code has been updated.

@matticbot
Copy link
Contributor

codestor4, Your synced wpcom patch D33973-code has been updated.

@wigglemuff
Copy link
Contributor Author

The latest change added a filter jetpack_fb_hide_cta which can be used to return false if someone wishes to display the Facebook's call to action button.

modules/widgets/facebook-likebox.php Outdated Show resolved Hide resolved
modules/widgets/facebook-likebox.php Outdated Show resolved Hide resolved
modules/widgets/facebook-likebox.php Outdated Show resolved Hide resolved
$like_args['show_faces'] = (bool) $like_args['show_faces'] ? 'true' : 'false';
$like_args['stream'] = (bool) $like_args['stream'] ? 'timeline' : 'false';
$like_args['cover'] = (bool) $like_args['cover'] ? 'false' : 'true';
$like_args['small_header'] = (bool) $like_args['small_header'] ? 'true' : 'false';
Copy link
Member

Choose a reason for hiding this comment

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

What's the current default value for this header? If I update Jetpack, will my header change unless I go to the widget options and toggle this new option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this by adding a value of true to small_header to get_default_args function. I think that's what you wanted here but let me know if I'm wrong 😅

Copy link
Member

Choose a reason for hiding this comment

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

I think it should default to false, since that's the default value in the Page Plugin. The idea is for the widget to look the same when folks will update Jetpack, so the large header has to stay until you decide to have a smaller header instead.

@matticbot
Copy link
Contributor

codestor4, Your synced wpcom patch D33973-code has been updated.

@wigglemuff wigglemuff added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 25, 2020
$like_args['show_faces'] = (bool) $like_args['show_faces'] ? 'true' : 'false';
$like_args['stream'] = (bool) $like_args['stream'] ? 'timeline' : 'false';
$like_args['cover'] = (bool) $like_args['cover'] ? 'false' : 'true';
$like_args['small_header'] = (bool) $like_args['small_header'] ? 'true' : 'false';
Copy link
Member

Choose a reason for hiding this comment

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

I think it should default to false, since that's the default value in the Page Plugin. The idea is for the widget to look the same when folks will update Jetpack, so the large header has to stay until you decide to have a smaller header instead.

*
* @param bool True value hides the call to action button
*/
$hide_cta = apply_filters( 'jetpack_facebook_likebox_hide_cta', true );
Copy link
Member

Choose a reason for hiding this comment

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

I would default to false here as well, for the same reasons as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right. Sorry, I checked now and the default is indeed false for both small_header and hide_cta - https://developers.facebook.com/docs/plugins/page-plugin/ I'll make this change now.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 25, 2020
@matticbot
Copy link
Contributor

codestor4, Your synced wpcom patch D33973-code has been updated.

@matticbot
Copy link
Contributor

codestor4, Your synced wpcom patch D33973-code has been updated.

@wigglemuff wigglemuff added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 27, 2020
@jeherve jeherve added this to the 8.4 milestone Feb 27, 2020
kraftbj
kraftbj previously approved these changes Mar 6, 2020
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Thanks for these updates. This looks good to me now.

@kraftbj kraftbj 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 Mar 6, 2020
@matticbot
Copy link
Contributor

codestor4, Your synced wpcom patch D33973-code has been updated.

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 should be good to go now!

@jeherve jeherve merged commit 8c01e22 into master Mar 17, 2020
@jeherve jeherve deleted the fix/fb-widget branch March 17, 2020 13:20
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 17, 2020
@jeherve
Copy link
Member

jeherve commented Mar 17, 2020

r204387-wpcom

jeherve added a commit that referenced this pull request Mar 23, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extra Sidebar Widgets Good For Community [Pri] Normal Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Facebook Widget: Expand widget functionality
5 participants