Add new sharing's variables by yogasukma · Pull Request #5404 · Automattic/jetpack

yogasukma · 2016-10-24T13:42:42Z

Fixes #5184

Changes proposed in this Pull Request:

Adding new 3 variables for sharing purposes.

%post_id% for getting post id
%site_url% for getting root url
%post_slug% for getting slug

Testing instructions:

enable sharing modules.
go to settings > sharings.
click on "add new services"
insert the custom format like %site_url%/read_offline/%post_slug% to get something like http://example.com/read_offline/hello-world

Screenshot when adding new services. notice all new variable are listed there.

cc @jeherve

jeherve · 2016-10-25T09:35:32Z

modules/sharedaddy/sharing-sources.php

 		$url = str_replace( '%post_url%', rawurlencode( $this->get_share_url( $post->ID ) ), $url );
 		$url = str_replace( '%post_full_url%', rawurlencode( get_permalink( $post->ID ) ), $url );
 		$url = str_replace( '%post_title%', rawurlencode( $this->get_share_title( $post->ID ) ), $url );
+		$url = str_replace( '%site_url%', rawurlencode( site_url() ), $url );


I'd recommend using home_url() instead. site_url() returns the location of the WordPress files, not necessarily the site's home page.

jeherve · 2016-10-25T09:38:10Z

modules/sharedaddy/sharing.php

 									<?php foreach ( $enabled['hidden'] as $id => $service ) {
 											$this->output_preview( $service );
-										}?>
+}?>


This indentation doesn't seem correct.

jeherve · 2016-10-25T09:38:36Z

modules/sharedaddy/sharing.php

-						<?php	$br = true; endforeach; ?>
+						<?php if ( $br ) { echo '<br />';} ?><label><input type="checkbox"<?php checked( in_array( $show, $global['show'] ) ); ?> name="show[]" value="<?php echo esc_attr( $show ); ?>" /> <?php echo esc_html( $label ); ?></label>
+						<?php	$br = true;
+endforeach; ?>


Indentation seems off here.

…bles

yogasukma · 2016-10-25T10:39:03Z

Ok @jeherve i changed to home_url rather than site_url and fixing indentation issues.

btw, how do you know and found for indentation issue? do you just manually quick read the code, or there is a tool for that?

I used phpcbf for fixing non wordpress standard code, and i thought it fixed the indentation too, but still some place need manual work to fix the indent.

jeherve · 2016-10-25T10:42:34Z

Thank you!

btw, how do you know and found for indentation issue? do you just manually quick read the code, or there is a tool for that?

I checked manually.

zinigor

Tested the changes, everything works as expected, but I have added some minor code style comments. Can you please fix them so we can merge?
Thank you!

zinigor · 2016-11-09T12:35:58Z

modules/sharedaddy/sharing-sources.php

+		if ( ! empty( $query ) ) {
+			if ( false === stripos( $url, '?' ) ) {
+				$url .= '?' . $query;
+			} else { $url .= '&amp;' . $query;


Minor style issue: please carry the statement to the next line as with the first block.

zinigor · 2016-11-09T12:38:09Z

modules/sharedaddy/sharing-sources.php

 			$this->smart = true;
-		else
-			$this->smart = false;
+		} else { $this->smart = false;


Same thing as the previous else statement please: new line after curly brace.

zinigor · 2016-11-09T12:39:54Z

modules/sharedaddy/sharing-sources.php

 	<div class="response-title"><?php _e( 'This post has been shared!', 'jetpack' ); ?></div>
- 	<div class="response-sub"><?php printf( __( 'You have shared this post with %s', 'jetpack' ), esc_html( $target_email ) ); ?></div>
- 	<div class="response-close"><a href="#" class="sharing_cancel"><?php _e( 'Close', 'jetpack' ); ?></a></div>
+		<div class="response-sub"><?php printf( __( 'You have shared this post with %s', 'jetpack' ), esc_html( $target_email ) ); ?></div>


If we're fixing indentation here, let's align these div elements on the same level because they are siblings.

zinigor · 2016-11-09T12:40:18Z

modules/sharedaddy/sharing-sources.php

+		<div class="response-close"><a href="#" class="sharing_cancel"><?php _e( 'Close', 'jetpack' ); ?></a></div>
 </div>
 <?php
+				} else { 					wp_safe_redirect( get_permalink( $post->ID ) . '?shared=email' );


Newline after curly brace please.

zinigor · 2016-11-09T12:40:24Z

modules/sharedaddy/sharing-sources.php

-					wp_safe_redirect( get_permalink( $post->ID ).'?shared=email' );

 				die();
+			} else { 				$error = 2;   // Email check failed


Newline after curly brace please.

zinigor · 2016-11-09T12:44:14Z

modules/sharedaddy/sharing-sources.php

 				s.type = "text/javascript";
 				s.async = true;
-				<?php if ( $jetpack_pinit_over ) echo "s.setAttribute('data-pin-hover', true);"; ?>
+				<?php if ( $jetpack_pinit_over ) { echo "s.setAttribute('data-pin-hover', true);";} ?>


It's going to look better if that condition is split into three lines, don't you think?

zinigor · 2016-11-09T12:45:07Z

modules/sharedaddy/sharing.php

 					</td>
 					<td class="services" id="share-drop-target">
-							<h2 id="drag-instructions" <?php if ( count( $enabled['visible'] ) > 0 ) echo ' style="display: none"'; ?>><?php _e( 'Drag and drop available services here.', 'jetpack' ); ?></h2>
+							<h2 id="drag-instructions" <?php if ( count( $enabled['visible'] ) > 0 ) { echo ' style="display: none"';} ?>><?php _e( 'Drag and drop available services here.', 'jetpack' ); ?></h2>


Same here, let's split the condition into several lines.

zinigor · 2016-11-09T12:45:21Z

modules/sharedaddy/sharing.php

 					</td>
 					<td class="services">
-						<h2<?php if ( count( $enabled['all'] ) > 0 ) echo ' style="display: none"'; ?>><?php _e( 'Sharing is off. Add services above to enable.', 'jetpack' ); ?></h2>
+						<h2<?php if ( count( $enabled['all'] ) > 0 ) { echo ' style="display: none"';} ?>><?php _e( 'Sharing is off. Add services above to enable.', 'jetpack' ); ?></h2>


Condition into multiple lines please.

@zinigor i think multiple line condition are not work here, since it will looks little mess. The context of condition are inside tag parameter. it will looks like this

`<h2
0 ) { echo ' style="display: none"'; } ?>>`
do you think it's ok?

Fair enough, let's convert it all into ternary operators, what do you say? Like this:

$selected = 'selected="selected"'; $hidden = ' style="display: none"'; ?> ... <h2 <?php echo count( $enabled['all'] ) > 0 ? $hidden : '' ?> ... <option<?php echo 'icon-text' === $global['button_style']' ? $selected : '' ?>

zinigor · 2016-11-09T12:46:51Z

modules/sharedaddy/sharing.php

-								<option<?php if ( $global['button_style'] == 'icon' ) echo ' selected="selected"';?> value="icon"><?php _e( 'Icon only', 'jetpack' ); ?></option>
-								<option<?php if ( $global['button_style'] == 'text' ) echo ' selected="selected"';?> value="text"><?php _e( 'Text only', 'jetpack' ); ?></option>
-								<option<?php if ( $global['button_style'] == 'official' ) echo ' selected="selected"';?> value="official"><?php _e( 'Official buttons', 'jetpack' ); ?></option>
+								<option<?php if ( $global['button_style'] == 'icon-text' ) { echo ' selected="selected"';}?> value="icon-text"><?php _e( 'Icon + text', 'jetpack' ); ?></option>


Same here, multiple lines for each conditional please.

same as above, in this context, one liner conditional makes it easy to read, this is a gist when it converted into multiple lines https://gist.github.com/ysupr/c02065f3af3069ea3ee11212485bfe94 what do you think @zinigor

zinigor · 2016-11-09T12:47:25Z

modules/sharedaddy/sharing.php

+										$label = $post_type_object->labels->name;
+									}
+								?>
+								<?php if ( $br ) { echo '<br />';} ?>


Multiple lines for the conditional please.

yogasukma · 2016-11-09T13:13:24Z

Ok will check it

samhotchkiss · 2016-11-09T19:36:31Z

Removed from milestone until style updates are done

yogasukma · 2016-11-10T21:57:08Z

@zinigor all code style feedback has been applied

ebinnion

Throughout the file, there are now several places where the code is using spaces instead of tabs for indentation.

Would you mind fixing these @ysupr before me merge?

ebinnion · 2016-12-07T18:36:01Z

modules/sharedaddy/sharing-sources.php

 				$url .= '?' . $query;
-			} else { $url .= '&amp;' . $query;
+            } else { 
+                $url .= '&amp;' . $query;


We're using spaces instead of tabs here, so the formatting looks off.

ebinnion · 2016-12-07T18:36:26Z

modules/sharedaddy/sharing-sources.php

 	<div class="response-title"><?php _e( 'This post has been shared!', 'jetpack' ); ?></div>
-		<div class="response-sub"><?php printf( __( 'You have shared this post with %s', 'jetpack' ), esc_html( $target_email ) ); ?></div>
-		<div class="response-close"><a href="#" class="sharing_cancel"><?php _e( 'Close', 'jetpack' ); ?></a></div>
+    <div class="response-sub"><?php printf( __( 'You have shared this post with %s', 'jetpack' ), esc_html( $target_email ) ); ?></div>


We should use tabs instead of spaces here.

ebinnion · 2016-12-07T18:36:47Z

modules/sharedaddy/sharing-sources.php

 				die();
-			} else { 				$error = 2;   // Email check failed
+            } else { 
+                $error = 2;   // Email check failed


Let's us tabes instead of spaces here.

ebinnion · 2016-12-07T18:36:58Z

modules/sharedaddy/sharing-sources.php

-		<div class="response-sub"><?php printf( __( 'You have shared this post with %s', 'jetpack' ), esc_html( $target_email ) ); ?></div>
-		<div class="response-close"><a href="#" class="sharing_cancel"><?php _e( 'Close', 'jetpack' ); ?></a></div>
+    <div class="response-sub"><?php printf( __( 'You have shared this post with %s', 'jetpack' ), esc_html( $target_email ) ); ?></div>
+    <div class="response-close"><a href="#" class="sharing_cancel"><?php _e( 'Close', 'jetpack' ); ?></a></div>


Same here. Tabs instead of spaces.

ebinnion · 2016-12-07T18:37:24Z

modules/sharedaddy/sharing-sources.php

 			$this->smart = true;
-		} else { $this->smart = false;
+        } else { 
+            $this->smart = false;


Tabs instead of spaces.

ebinnion · 2016-12-07T18:38:14Z

modules/sharedaddy/sharing-sources.php

 			$this->smart = true;
-		} else { $this->smart = false;
+        } else { 
+            $this->smart = false;


Tabs instead of spaces.

ebinnion · 2016-12-07T18:41:39Z

modules/sharedaddy/sharing-sources.php

 				s.type = "text/javascript";
 				s.async = true;
-				<?php if ( $jetpack_pinit_over ) { echo "s.setAttribute('data-pin-hover', true);";} ?>
+                <?php if ( $jetpack_pinit_over ) { 


Tabs instead of spaces.

Also, I believe the echo should be indented once more than the conditional.

Lastly, I prefer the following style, although yours works just as well.

<?php if ( $jetpack_pinit_over ): echo "s.setAttribute('data-pin-hover', true);"; endif; ?>

ebinnion · 2016-12-07T18:41:54Z

modules/sharedaddy/sharing-sources.php

 			$this->smart = true;
-		} else { $this->smart = false;
+        } else { 
+            $this->smart = false;


Tabs instead of spaces.

ebinnion · 2016-12-07T18:42:04Z

modules/sharedaddy/sharing.php

 					</td>
 					<td class="services" id="share-drop-target">
-							<h2 id="drag-instructions" <?php if ( count( $enabled['visible'] ) > 0 ) { echo ' style="display: none"';} ?>><?php _e( 'Drag and drop available services here.', 'jetpack' ); ?></h2>
+                            <h2 id="drag-instructions" <?php if ( count( $enabled['visible'] ) > 0 ) { echo ' style="display: none"';} ?>><?php _e( 'Drag and drop available services here.', 'jetpack' ); ?></h2>


Tabs instead of spaces.

ebinnion · 2016-12-07T18:42:13Z

modules/sharedaddy/sharing.php

 									}
 								?>
-								<?php if ( $br ) { echo '<br />';} ?>
+                                <?php 


Tabs instead of spaces.

samhotchkiss · 2016-12-08T19:33:29Z

Merged in #5832

Thanks @ysupr !

yogasukma added 3 commits October 24, 2016 21:30

add post_id, post_slug, and site_url to sharing variables

ec9ff7d

run phpcbf to check the files

ae903d0

fixing conflict

1d547ce

jeherve requested changes Oct 25, 2016

View reviewed changes

jeherve added this to the 4.4 milestone Oct 25, 2016

yogasukma added 3 commits October 25, 2016 17:54

Merge remote-tracking branch 'upstream/master' into add/sharing-varia…

0e8dfa7

…bles

changes site_url to home_url and some indentation

8e2ad86

indentation

34e1fa5

jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Oct 25, 2016

zinigor suggested changes Nov 9, 2016

View reviewed changes

samhotchkiss modified the milestones: Not Currently Planned, 4.4 Nov 9, 2016

jeherve added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Nov 9, 2016

yogasukma added 2 commits November 10, 2016 05:20

fixing some code style based on feedback by @zinigor

39e0735

using ternary conditional to match code style

4c0efe2

samhotchkiss added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Nov 11, 2016

samhotchkiss removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Nov 11, 2016

samhotchkiss modified the milestones: 4.5, Not Currently Planned Nov 11, 2016

ebinnion requested changes Dec 7, 2016

View reviewed changes

samhotchkiss mentioned this pull request Dec 8, 2016

Ysupr add/sharing variables #5832

Merged

samhotchkiss closed this Dec 8, 2016

samhotchkiss removed the [Status] Needs Review This PR is ready for review. label Dec 8, 2016

yogasukma deleted the add/sharing-variables branch January 18, 2017 06:30

Conversation

yogasukma commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogasukma commented Oct 25, 2016

Uh oh!

jeherve commented Oct 25, 2016

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogasukma Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogasukma Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogasukma commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samhotchkiss commented Nov 9, 2016

Uh oh!

yogasukma commented Nov 10, 2016

Uh oh!

ebinnion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samhotchkiss commented Dec 8, 2016

yogasukma commented Oct 24, 2016 •

edited

Loading

yogasukma Nov 9, 2016 •

edited

Loading

yogasukma Nov 9, 2016 •

edited

Loading

yogasukma commented Nov 9, 2016 •

edited

Loading