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

use content, attribute 0 -or- 'id' #284

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@rsrchboy

rsrchboy commented Mar 1, 2014

There are a number of other gist-shortcode-providing plugins out there, almost
all of which use the 'id' attribute. This change honors that attribute as
well, to allow for an easy migration.

use content, attribute 0 -or- 'id'
There are a number of other gist-shortcode-providing plugins out there, almost
all of which use the 'id' attribute.  This change honors that attribute as
well, to allow for an easy migration.
@rsrchboy

This comment has been minimized.

Show comment
Hide comment
@rsrchboy

rsrchboy Mar 4, 2014

FWIW, I'd tag this as more urgent than just an enhancement.. Unless someone is willing to go in and edit the jetpack plugin, anyone on 2.9 and using any sort of embedded gists via any of the other ``[gist]` shortcode-providing plugins is going to be force to make a choice between having all their gists broken, manually changing all the embed shortcodes in each post using them, or disabling all of jectpack's functionality.

rsrchboy commented Mar 4, 2014

FWIW, I'd tag this as more urgent than just an enhancement.. Unless someone is willing to go in and edit the jetpack plugin, anyone on 2.9 and using any sort of embedded gists via any of the other ``[gist]` shortcode-providing plugins is going to be force to make a choice between having all their gists broken, manually changing all the embed shortcodes in each post using them, or disabling all of jectpack's functionality.

@kraftbj kraftbj added this to the 3.0 Freeze milestone Mar 4, 2014

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis Mar 4, 2014

Member

Two changes before I'd be okay merging it in -- 1) make sure you quote strings, for example id as an array key, and 2) don't nest ternary conditionals like that, it gets really illegible. Do sequential if checks instead.

Member

georgestephanis commented Mar 4, 2014

Two changes before I'd be okay merging it in -- 1) make sure you quote strings, for example id as an array key, and 2) don't nest ternary conditionals like that, it gets really illegible. Do sequential if checks instead.

@georgestephanis

This comment has been minimized.

Show comment
Hide comment
@georgestephanis

georgestephanis Mar 4, 2014

Member

Also, in lines 19-20, it looks like you've inadvertently swapped out tab-based indentation for spaces?

Member

georgestephanis commented Mar 4, 2014

Also, in lines 19-20, it looks like you've inadvertently swapped out tab-based indentation for spaces?

@blobaugh

This comment has been minimized.

Show comment
Hide comment
@blobaugh

blobaugh Mar 4, 2014

Contributor

@rsrchboy slight bunny trail, but I wanted to make sure you have some additional information. Each shortcode in Jetpack passes through the jetpack_shortcodes_to_include filter. If someone has enabled the Jetpack shortcodes module and ever needs to turn one off they can tap into that filter.

https://github.com/Automattic/jetpack/blob/master/modules/shortcodes.php#L49

Contributor

blobaugh commented Mar 4, 2014

@rsrchboy slight bunny trail, but I wanted to make sure you have some additional information. Each shortcode in Jetpack passes through the jetpack_shortcodes_to_include filter. If someone has enabled the Jetpack shortcodes module and ever needs to turn one off they can tap into that filter.

https://github.com/Automattic/jetpack/blob/master/modules/shortcodes.php#L49

@blobaugh blobaugh added Multisite and removed Multisite labels Mar 25, 2014

@blobaugh

This comment has been minimized.

Show comment
Hide comment
@blobaugh

blobaugh Mar 25, 2014

Contributor

@rsrchboy There has been no update on this in 21 days. Is this something you would like to fix for inclusion? This PR will be closed soon if we do not hear back.

Contributor

blobaugh commented Mar 25, 2014

@rsrchboy There has been no update on this in 21 days. Is this something you would like to fix for inclusion? This PR will be closed soon if we do not hear back.

@blobaugh

This comment has been minimized.

Show comment
Hide comment
@blobaugh

blobaugh May 16, 2014

Contributor

Closing due to lack of response to questions for original poster.

Contributor

blobaugh commented May 16, 2014

Closing due to lack of response to questions for original poster.

@blobaugh blobaugh closed this May 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment