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

Add a filter to modify Liveblog output location #531

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alexiskulash
Copy link

Fixes: #525

Some users would prefer having Liveblog output at the top of their posts rather than the bottom. This filter introduces the simple functionality to choose whether the Liveblog output should be above content or below it (default).

Fixes: #525

Some users would prefer having Liveblog output at the top of their posts rather than the bottom. This filter introduces the simple functionality to choose whether the Liveblog output should be above the content or below it (default).
@alexiskulash alexiskulash self-assigned this Sep 26, 2018
liveblog.php Outdated
@@ -1169,6 +1169,10 @@ public static function add_liveblog_to_content( $content ) {
$liveblog_output = '<div id="wpcom-liveblog-container" class="' . self::$post_id . '"></div>';

$liveblog_output = apply_filters( 'liveblog_add_to_content', $liveblog_output, $content, self::$post_id );

if ( false === apply_filters( 'liveblog_output_at_bottom', true ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why but to me it feels more obvious to have the filter that changes something to be "liveblog_output_at_top" and have it default to false. I'm having a hard time to vocalize it but it seems like a negation that doesn't need to be there. Like the question is liveblog_output_at_bottom and then having it to true will will then output it to the top. It works, but it "feels" strange to me.

Copy link
Author

Choose a reason for hiding this comment

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

I think that makes sense; it kind of feels like a double negative when folks want to actually have output at the top, whereas a default false for 'liveblog_output_at_top' feels more straightforward.

…f a user wants to place Liveblog output at the top of their post content, they can override the default false by having the filter 'liveblog_output_at_top' return true.
@GaryJones
Copy link
Contributor

The failure here appears to be related to the version number that was in packages.json. It was changed here. Perhaps it was because 1.9.0-RC2 not a published release?

I suspect that re-starting the build will make this pass.

@GaryJones
Copy link
Contributor

Restarting the build produced the same result, but I think rebasing it onto the latest master may help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

LiveBlog at the top of the article
4 participants