Handles custom tab sizes for gist embeds and shortcodes #13807

ChaosExAnima · 2019-10-22T21:08:58Z

Changes proposed in this Pull Request:

This adds functionality for tab sizing for gist embeds and shortcodes via ?ts= query param.
Adds ts attribute to shortcodes.

Known limitation: Currently, AMP does not have a way to change tab sizing. This is a limitation of the AMP amp-gist element.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This adds functionality to the embed/shortcode module.

Testing instructions:

Enable Shortcode Embeds module.
Create a post with a gist embed that appends ?ts=4 to the gist URL.
Create a post with a gist shortcode that has ts=4 in it.

Screenshot:

Proposed changelog entry for your changes:

Adds support for tab sizing for gist embeds.

matticbot · 2019-10-22T21:09:04Z

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

ChaosExAnima · 2019-10-22T21:11:07Z

modules/shortcodes/gist.php

@@ -156,6 +166,12 @@ function github_gist_shortcode( $atts, $content = '' ) {
 		$file = rawurlencode( $file );
 	}

+	// Set the tab size, allowing attributes to override the query string.
+	$tab_size = $gist_info['ts'];
+	if ( ! empty( $atts['ts'] ) ) {


I decided to allow the attributes to override the query string to allow for theme filtering via shortcode_atts_{$shortcode} and similar.

Should we add some default attributes in shortcode_atts at the top of github_gist_shortcode to allow for this to happen?

The code already sets the defaults in jetpack_gist_get_shortcode_id. Do you feel that it needs to have that set additionally?

We do indeed have defaults but we do not have the shortcode_atts function that would make the shortcode_atts_gist filter you mentioned above available, iirc. Maybe we could add that function here, to make the filter available?

Something like this?

jetpack/modules/shortcodes/recipe.php

Line 175 in 89a9af9

$atts = shortcode_atts(

That's not necessarily a blocker here. 👍

ChaosExAnima · 2019-10-22T21:11:39Z

modules/shortcodes/gist.php

@@ -195,7 +211,11 @@ class_exists( 'Jetpack_AMP_Support' )
 	);

 	// inline style to prevent the bottom margin to the embed that themes like TwentyTen, et al., add to tables.
-	$return = '<style>.gist table { margin-bottom: 0; }</style><div class="gist-oembed" data-gist="' . esc_attr( $id ) . '"></div>';
+	$return = sprintf(
+		'<style>.gist table { margin-bottom: 0; }</style><div class="gist-oembed" data-gist="%1$s" data-ts="%2$d"></div>',


Noting that we embed this multiple times. May be a better way to do this.

jetpackbot · 2019-10-22T21:12:46Z

	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 4de9a1d

jeherve

What do you think about adding a new Unit Test to cover this use-case to our existing Gist tests here:
tests/php/modules/shortcodes/test-class.gist.php

modules/shortcodes/gist.php

matticbot · 2019-10-23T15:18:22Z

ChaosExAnima, Your synced wpcom patch D34348-code has been updated.

Resolves bugs with older tests that were failing with tab spacing code.

Test had extraneous whitespace, which let to WP parsing `/]` as closing of tag. Changed method of slash trimming to use `trim` and fixed test.

matticbot · 2019-10-23T17:48:13Z

ChaosExAnima, Your synced wpcom patch D34348-code has been updated.

jeherve · 2019-10-24T08:55:30Z

modules/shortcodes/gist.php

@@ -156,6 +166,12 @@ function github_gist_shortcode( $atts, $content = '' ) {
 		$file = rawurlencode( $file );
 	}

+	// Set the tab size, allowing attributes to override the query string.
+	$tab_size = $gist_info['ts'];
+	if ( ! empty( $atts['ts'] ) ) {


We do indeed have defaults but we do not have the shortcode_atts function that would make the shortcode_atts_gist filter you mentioned above available, iirc. Maybe we could add that function here, to make the filter available?

Something like this?

jetpack/modules/shortcodes/recipe.php

Line 175 in 89a9af9

$atts = shortcode_atts(

That's not necessarily a blocker here. 👍

ChaosExAnima · 2019-10-24T14:29:44Z

@jeherve - Did a bit of tinkering, and it looks like adding shortcode_atts will actually break everything as the code uses empty a lot instead of setting defaults. I'd suggest releasing this iteration now and making a cleanup ticket to rectify the issues there?

jeherve · 2019-10-24T14:43:29Z

That works for me!

* 7.9: Changelog * Update version number * Update stable tag and tested up to * Changelog: add #13530 * changelog: add #13578 * Changelog: add #13598 * Changelog: add entry for numerous block preview changes * Changelog: add #13599 * changelog: add #13541 * Changelog: add #13542 * Changelog: add #13331 * Changelog: add #13558 * Changelog: add #13409 * Changelog: add #13582 * Changelog: add #13600 * Changelog: add #13601 * Changelog: add #13595 * Changelog: add #12695 * Changelog: add #13009 * Changelog: add #13649 * Changelog: add #13450 * Changelog: add #13507 * Changelog: add #13658 * Changelog: add #13687 * changelog: add #13683 * Changelog: add #9323 * Changelog: add #13681 * Fix typos in readme * Add link to WordPress Beta Tester plugin * Changelog: add #13630 * Changelog: add #13695 * Changelog: add #13659 * Changelog: add #13716 * Changelog: add #13664 * Changelog: add #13682 * Changelog: add #13362 * Changelog: add #13563 * Add testing list for #13563 * Changelog: add #13735 * Changelog: add #13752 * Changelog: add #13624 * Changelog: add #13756 * Changelog: add #13745 * Changelog: add #13728 * Changelog: add #13779 * Changelog: add #13699 * Changelog: add #13804 * Changelog: add #13761 * Changelog: add #13637 * Changelog: add #13517 * Changelog: add #13521 * Changelog: add #13729 * Testing list: add testing instructions for #13729 * Changelog: add sync changes * Changelog: add #13807 * Changelog: add #13654 * Changelog: add #13795 * Changelog: add #13801 * Changelog: add #13818 * Changelog: add #13725 * Changelog: add #13831 * Changelog: add #13516 * Testing list: add Twenty Twenty instructions * Changelog: add #13799 * Changelog: add #13805 * Changelog: add #13688 * Changelog: add #13830

[not verified] Handles custom tab sizes for gist embeds and shortcodes

2ab9cd1

ChaosExAnima added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Shortcodes / Embeds [Status] Needs Review This PR is ready for review. [Pri] Low labels Oct 22, 2019

ChaosExAnima added this to the 7.9 milestone Oct 22, 2019

ChaosExAnima requested a review from a team October 22, 2019 21:08

ChaosExAnima self-assigned this Oct 22, 2019

matticbot added Touches WP.com Files labels Oct 22, 2019

ChaosExAnima commented Oct 22, 2019

View reviewed changes

jeherve requested changes Oct 23, 2019

View reviewed changes

modules/shortcodes/gist.php Show resolved Hide resolved

Makes tab size default consistent

55d9440

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

ChaosExAnima added 4 commits October 23, 2019 13:47

Fixes tests to no longer fail

4ca5709

Resolves bugs with older tests that were failing with tab spacing code.

[not verified] Fixes incorrect test and slash trimming method.

f36a598

Test had extraneous whitespace, which let to WP parsing `/]` as closing of tag. Changed method of slash trimming to use `trim` and fixed test.

[not verified] Ensures default tab size is set if function exits early

217bdfa

[not verified] Adds tests to cover tab size functionality

4de9a1d

ChaosExAnima added [Status] Needs Review This PR is ready for review. and removed [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. labels Oct 23, 2019

jeherve approved these changes Oct 24, 2019

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 24, 2019

ChaosExAnima mentioned this pull request Oct 24, 2019

Implement attribute filter for gist shortcode #13833

Open

kraftbj merged commit f6e41c8 into master Oct 24, 2019

kraftbj deleted the update/10534-gist-tab-size branch October 24, 2019 21:25

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 24, 2019

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Oct 28, 2019

jeherve added a commit that referenced this pull request Oct 28, 2019

Changelog: add #13807

988b822

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handles custom tab sizes for gist embeds and shortcodes #13807

Handles custom tab sizes for gist embeds and shortcodes #13807

ChaosExAnima commented Oct 22, 2019 •

edited by jeherve

Loading

matticbot commented Oct 22, 2019

ChaosExAnima Oct 22, 2019

jeherve Oct 23, 2019

ChaosExAnima Oct 23, 2019

jeherve Oct 24, 2019

ChaosExAnima Oct 22, 2019

jetpackbot commented Oct 22, 2019 •

edited

Loading

jeherve left a comment

matticbot commented Oct 23, 2019

matticbot commented Oct 23, 2019

jeherve Oct 24, 2019

ChaosExAnima commented Oct 24, 2019

jeherve commented Oct 24, 2019

Handles custom tab sizes for gist embeds and shortcodes #13807

Handles custom tab sizes for gist embeds and shortcodes #13807

Conversation

ChaosExAnima commented Oct 22, 2019 • edited by jeherve Loading

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Oct 22, 2019

ChaosExAnima Oct 22, 2019

Choose a reason for hiding this comment

jeherve Oct 23, 2019

Choose a reason for hiding this comment

ChaosExAnima Oct 23, 2019

Choose a reason for hiding this comment

jeherve Oct 24, 2019

Choose a reason for hiding this comment

ChaosExAnima Oct 22, 2019

Choose a reason for hiding this comment

jetpackbot commented Oct 22, 2019 • edited Loading

jeherve left a comment

Choose a reason for hiding this comment

matticbot commented Oct 23, 2019

matticbot commented Oct 23, 2019

jeherve Oct 24, 2019

Choose a reason for hiding this comment

ChaosExAnima commented Oct 24, 2019

jeherve commented Oct 24, 2019

ChaosExAnima commented Oct 22, 2019 •

edited by jeherve

Loading

jetpackbot commented Oct 22, 2019 •

edited

Loading