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

Social Media Icons Widget: add Google+ #2654

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
6 participants
@brendancarr
Copy link

brendancarr commented Sep 2, 2015

Add a Google+ link to the normal social icons - technically not a username, but kept the variable naming convention. Uses the correct genericon to go along with it.

brendancarr added some commits Sep 2, 2015

Update social-media-icons.php
Add a Google+ link to the normal social icons - technically not a username, but kept the variable naming convention. Uses the correct genericon to go along with it.
*
* @since 3.7.0
*/
do_action( 'jetpack_social_media_icons_widget_list_before' );

This comment has been minimized.

Copy link
@dereksmart

dereksmart Sep 2, 2015

Member

Why are you removing these actions?

This comment has been minimized.

Copy link
@brendancarr

brendancarr Sep 2, 2015

Author

Sorry, my bad, I re-did it after reviewing another pull request and noting that it needed a way to differentiate between numbered pages and custom G+ Urls. I added that and accidentally removed some other stuff... I will re-do and resubmit!

This comment has been minimized.

Copy link
@dereksmart

dereksmart Sep 2, 2015

Member

awesome, thanks!

if ( ! empty( $instance['google_username'] ) ) {
$title_text = (is_numeric( $instance['google_username'] ) ? $alt_text_numeric : $alt_text);
$html .= '<li><a title="' . sprintf( $title_text, esc_attr( $instance['google_username'] ), 'Google+' ) . '" href="' . esc_url( 'https://plus.google.com/u/1/' . $instance['google_username'] . '/' ) . '" class="genericon genericon-googleplus" target="_blank"><span class="screen-reader-text">' . sprintf( $alt_text, esc_html( $instance['google_username'] ), 'Google+' ) . '</span></a></li>';

This comment has been minimized.

Copy link
@dereksmart

dereksmart Sep 2, 2015

Member

My personal google+ url is https://plus.google.com/u/0/, which differs from this.

Also, we'll need to find a way to play nice with custom usernames, which has a https://plus.google.com/+username/posts syntax

This comment has been minimized.

Copy link
@brendancarr

brendancarr Sep 2, 2015

Author

I just checked - it doesn't matter whether it's a 0 or 1, that's based on how many accounts you have signed in. If it gets changed to a 0 it will still go to the same page.
I also replaced it with my old one, +BrendanCarr84, and it went to the correct page.

This comment has been minimized.

Copy link
@dereksmart

dereksmart Sep 2, 2015

Member

Sweet! Do you think it'll be obvious enough that they need to include the +?

This comment has been minimized.

Copy link
@brendancarr

brendancarr Sep 2, 2015

Author

Good point. Give me 5 minutes to add a check for the first character.

This comment has been minimized.

Copy link
@brendancarr

brendancarr Sep 2, 2015

Author

Lines 123/124 are modified to take a missing + into account now too.

Update social-media-icons.php
Fixed missing actions - line 122 checks for a numeric G+ page and adjusts the title text accordingly, to the options on line 77/78
@brendancarr

This comment has been minimized.

Copy link
Author

brendancarr commented Sep 2, 2015

OK, this is the live code I use on my site currently - brendancarr.ca - it's a very basic check for whether the username is a number or not, but it should do the trick.

brendancarr added some commits Sep 2, 2015

Update social-media-icons.php
Added check on line 123 for + at the start of a non-numeric string, and amended line 124 to add it to the link when required
@@ -116,6 +118,12 @@ public function widget( $args, $instance ) {
$html .= '<li><a title="' . sprintf( $alt_text, esc_attr( $instance['vimeo_username'] ), 'Vimeo' ) . '" href="' . esc_url( 'https://vimeo.com/' . $instance['vimeo_username'] . '/' ) . '" class="genericon genericon-vimeo" target="_blank"><span class="screen-reader-text">' . sprintf( $alt_text, esc_html( $instance['vimeo_username'] ), 'Vimeo' ) . '</span></a></li>';
}
if ( ! empty( $instance['google_username'] ) ) {
$title_text = (is_numeric( $instance['google_username'] ) ? $alt_text_numeric : $alt_text);
$plus = ( (!is_numeric( $instance['google_username']) && substr($instance['google_username'], 0,1 ) != "+" ) ? "+" : "" );

This comment has been minimized.

Copy link
@dereksmart

dereksmart Sep 2, 2015

Member

Nitpick code style: some missing spaces between parentheses in this line and the one above it.

This comment has been minimized.

Copy link
@brendancarr

brendancarr Sep 2, 2015

Author

Funny you should say that ... I didn't edit the github version but I did my own ... One sec.

@dereksmart

This comment has been minimized.

Copy link
Member

dereksmart commented Sep 2, 2015

Looking pretty good! Just one more nitpick about whitespace

@dereksmart dereksmart added this to the 3.8 milestone Sep 2, 2015

@dereksmart

This comment has been minimized.

Copy link
Member

dereksmart commented Sep 30, 2015

This LGTM

@samhotchkiss

This comment has been minimized.

Copy link
Contributor

samhotchkiss commented Oct 5, 2015

This looks good, however, @zinigor is in the process of overhauling this code, so I'll leave it up to him to get G+ added in, probably borrowing from your PR

@jeherve jeherve changed the title Update social-media-icons.php Social Media Icons Widget: add Google+ Oct 19, 2015

@zinigor

This comment has been minimized.

Copy link
Member

zinigor commented Oct 21, 2015

Thank you @brendancarr for this PR, I have incorporated your changes in #2741

@samhotchkiss

This comment has been minimized.

Copy link
Contributor

samhotchkiss commented Oct 21, 2015

Alright, I'm going ahead and closing this PR in favor of #2741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.