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

Switch to shortcodes #250

Merged
merged 26 commits into from
Jan 23, 2023
Merged

Conversation

toolstack
Copy link
Contributor

As discussed in #246, this PR converts the existing template codes to shortcodes. As well as add new shortcodes for:

  • [ap_hashcats] - The post's categories as hashtags
  • [ap_image] - The URL for the post's featured image, full size [ap_thumbnail] - The URL for the post's featured image thumbnail size
  • [ap_author] - The author's name
  • [ap_authorurl] - The URL to the author's profile page [ap_date] - The post's date
  • [ap_time] - The post's time
  • [ap_datetime] - The post's date/time formated as "date @ time" [ap_blogurl] - The URL to the site
  • [ap_blogname] - The name of the site
  • [ap_blogdesc] - The description of the site

Finally, it adds in a length parameter to the [ap_excerpt] shortcode.

A few items of note/discussion:

  • I chose the ap_ prefix for the shortcodes as a compromise between length and uniqueness. Two character prefixes aren't ideal, you can run into conflicts with other plugins, but likewise, the shortcodes should be easily typeable by the users.
  • The shortcodes are only active during the post processing, so they are not available during the loop or in the WordPress editor.
  • The current code 'updates' the custom template any time it is actually used, otherwise it remains as it was set by the user. This adds a bit of extra processing per item (assuming the custom template is in use), but seems like a reasonable compromise to make sure things don't break.
  • The new length parameter currently doesn't really work, it needs Make the excerpt code actually crop the excerpt at 400 characters. #247 to be in place first, as the exisiting code will apply the excerpt_length filter which is usually set by the theme.

Closes #246

As well as add new shortcodes for:

[ap_hashcats] - The post's categories as hashtags
[ap_image] - The URL for the post's featured image, full size
[ap_thumbnail] - The URL for the post's featured image thumbnail size
[ap_author] - The author's name
[ap_authorurl] - The URL to the author's profile page
[ap_date] - The post's date
[ap_time] - The post's time
[ap_datetime] - The post's date/time formated as "date @ time"
[ap_blogurl] - The URL to the site
[ap_blogname] - The name of the site
[ap_blogdesc] - The description of the site
@pfefferle
Copy link
Member

The shortcode does not seem to have the post context, if doing it that way. You can test it, by adding activitypub to the end of a post permalink. The shortcode will not be replaced.

I hacked together a simple (quick and dirty) version of how I thought it might work: #255

@toolstack
Copy link
Contributor Author

Seems to have been two problems:

  • For some reason the constructor for the class was being called after the content was being genreated, so the shortcodes weren't registered yet. Easy fix, just moved the add_shortcode() calls to the content function to make sure they exist.
  • The second was a brain fart by me, I was echoing out the shortcodes instead of returning them as a string.

Both issues fixed in latest commit.

I did consider creating a single funciton per shortcode, but that seemed like overkill for such simple shortcodes, as does creating a whole new class for them as they are only ever used in the post.

@toolstack
Copy link
Contributor Author

Oh, and there is something broken on the setup for adding activitypub to the end of a post permalink, as it doesn't work unless you update your permalink settings (even just saving them as they are).

@toolstack
Copy link
Contributor Author

@pfefferle I've integrated the ideas from #255 into this PR, creating a seperate class for the shortcodes.

I added the class to the root plugin file, but didn't call an init on it, instead leaving it as part of the post class code.

I also added a size attribute to the image shortcode.

Copy link
Member

@pfefferle pfefferle 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 all your work so far!

I have some feedback inline.

activitypub.php Show resolved Hide resolved
includes/model/class-post.php Show resolved Hide resolved
includes/class-shortcodes.php Outdated Show resolved Hide resolved
includes/class-shortcodes.php Outdated Show resolved Hide resolved
includes/class-shortcodes.php Outdated Show resolved Hide resolved
@adegans
Copy link

adegans commented Jan 23, 2023

Aren't shortcodes sort of reserved for use in widgets, post and page content?

@pfefferle
Copy link
Member

@adegans but why not re-use them here? otherwise we had to implement everything shortcodes already do.

@adegans
Copy link

adegans commented Jan 23, 2023

You can indeed 're-use' them. But think of the dumb users. While it may look familiar it may also prompt them to use [permalink] or whatever in the wrong places.

If you have a distinctly different tag like %permalink% which can work just as easy with a str_replace() or preg_replace() people won't try funny stuff.

As an example (or comparison), I've been using tags like that in my AdRotate plugin for over 10 years and it never had any confusion as it's a distinctly different tag from what people use in actual content fields.

Just something to consider.

@@ -81,8 +69,7 @@
<li><code>[ap_shortlink <i>type=xxx</i>]</code> - <?php echo \wp_kses( \__( 'The post\'s shortlink. I can recommend <a href="https://wordpress.org/plugins/hum/" target="_blank">Hum</a>, to prettify the Shortlinks. Type can be either: raw (the raw url, no escaping), esc (the html escaped url), html (default, an a tag to the url).', 'activitypub' ), 'default' ); ?></li>
<li><code>[ap_hashtags]</code> - <?php \esc_html_e( 'The post\'s tags as hashtags.', 'activitypub' ); ?></li>
<li><code>[ap_hashcats]</code> - <?php \esc_html_e( 'The post\'s categories as hashtags.', 'activitypub' ); ?></li>
<li><code>[ap_image <i>size=xxx</i>]</code> - <?php \esc_html_e( 'The URL for the post\'s featured image, defaults to full size. The size attribute can be any of the following:', 'activitypub' ); echo esc_html( $registered_sizes ); ?></li>
<li><code>[ap_thumbnail]</code> - <?php echo \wp_kses( sprintf( __( 'The URL for the post\'s featured image thumbnail size (%s).', 'activitypub'), $thumbnail_size ), 'default' ); ?></li>
<li><code>[ap_image <i>type=full</i>]</code> - <?php \esc_html_e( 'The URL for the post\'s featured image, defaults to full size. The type attribute can be any of the following: thumbnail, medium, large, full', 'activitypub' ); ?></li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest keeping the custom list, many sites add addtional sizes or may even remove standard ones.

@pfefferle
Copy link
Member

@adegans the problem is, that there are a lot requests from users that want to customize the output more than just some placeholders, like the size of an image, the length of an excerpt, ... and to implement that, I have to implement everything a shortcode does.

Copy link
Member

@pfefferle pfefferle left a comment

Choose a reason for hiding this comment

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

LGTM

@pfefferle pfefferle merged commit bd89066 into Automattic:master Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants