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 the featured media to the two-column classic post #1140

Merged
merged 16 commits into from
Apr 12, 2016

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Feb 17, 2016

Changes

  • copies the largo_hero call and the after-hero action from the single-column template
  • removes .hero styles from the body.normal selector, and sets them with no parent selector in less/inc/single.less
  • adds the 'classic' post template to the post types that will have largo_remove_hero run against them to prevent duplicate hero images in the event that the featured media image is the same as the image in the first paragraph of the story
  • adds a filter largo_remove_hero (and docs) that allow themes/plugins to prevent largo_remove_hero from removing duplicate heroes in posts. Passes the $post to the filtering function as well.
  • prevents largo_remove_hero from running if global $post is unset, because the function requires access to the global $post. This was problematic because the tests for largo_excerpt were running with a post ID but not setting a global $post. Since the test for largo_excerpt are designed to test that function and not largo_remove_hero, this PR fixes largo_remove_hero and not the test.
  • Stubs a bunch of tests for inc/post-templates.php, including docs for the conditions in which largo_remove_hero should return the original content passed to it. Does not write any new tests.

Things this does not change yet:

  • styles for heroes on two-column posts
  • compatibility testing with child themes

This is a Work In Progress pull request for #934, copying the code written for CC-5 into Largo for its eventual merge. We weren't doing this in Largo right then because of the need to check this against other child themes.

@benlk
Copy link
Collaborator Author

benlk commented Apr 5, 2016

In testing, many two-column sites will have used a different featured image from the beginning-of-story image. This function doesn't do anything to prevent two images next to each other at the beginning of the story. It just prevents two identical images next to each other.

We should discuss how to handle the two-adjacent-images situation. It might need to be on a per-site.

benlk added 2 commits April 5, 2016 14:15
@benlk
Copy link
Collaborator Author

benlk commented Apr 5, 2016

I think this is ready for review.

Here's an embedded iframe in the two-column hero position:

screen shot 2016-04-05 at 2 58 36 pm

A video on mobile:

screen shot 2016-04-05 at 2 59 51 pm

An image on a very narrow mobile device:
screen shot 2016-04-05 at 3 01 44 pm

Same small image on desktop:

screen shot 2016-04-05 at 3 02 22 pm

*
* @since 0.4 - in Largo's single-column template
* @since 0.5.5 - in Largo's two-column template
*
* @param String $content the post content passed in by WordPress filter
* @return String filtered post content.
*/
function largo_remove_hero($content) {

Choose a reason for hiding this comment

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

should the default be to run this or not run it? seems like maybe a decision that would need to be made by each site, individually

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The default is currently to run it (in single-column posts), so if we changed the default, it would affect many sites in production.

For sites that are moving to Largo, they can definitely make the decision at migration time, which is why the filter was added.

@@ -239,9 +283,9 @@ function largo_url_to_attachmentid($url) {
* @filter largo_partial_by_post_type
* @since 0.5.4
*/
function largo_get_partial_by_post_type($partial, $post_type, $context) {
function largo_get_partial_by_post_type( $partial, $post_type, $context ) {
// Remove this conditional in #926.

Choose a reason for hiding this comment

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

can this be safely removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would rather do all the #926 stuff in its own PR.

Speaking of #926, why is it closed? As far as I know, we never actually went through and removed any of the argolinks code.

Choose a reason for hiding this comment

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

afaik #926 is already complete, do you see anywhere where this is still an issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inc/ajax-functions.php:195:             // argolinks post type
inc/ajax-functions.php:196:             $partial = ( get_post_type() == 'argolinks' ) ? 'argolinks' : $partial;
inc/post-templates.php:288:     if ( $post_type == 'argolinks' ) {
inc/post-templates.php:289:             $partial = 'argolinks';
tests/inc/test-ajax-functions.php:66:           // @todo find way to test get_post_type() returning argolinks.
tests/inc/test-ajax-functions.php:115:          // @todo find way to test get_post_type() returning argolinks.

Some docs, some LMP partial stuff. It's not actually that big a problem.

@benlk
Copy link
Collaborator Author

benlk commented Apr 11, 2016

Merge conflicts now resolved.

@aschweigert
Copy link

do we know why test_insert_image_no_thumb is failing?

@benlk
Copy link
Collaborator Author

benlk commented Apr 11, 2016

No, I don't.


$p = explode("\n",$content);
$p = explode( '\n', $content );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is part of the reason the tests are now failing: This needs to be double quotes, to get the explode to work on newlines.

@benlk
Copy link
Collaborator Author

benlk commented Apr 11, 2016

One of the "code style" commits replaced "\n" with '\n', which meant that the largo_hero_remove function was exploding and imploding the post on the literal string \n instead of on newlines. This broke the tests, which were looking for newlines in the output instead of escaped \n.

The prominence term update test failures have been around since we merged #1116, I believe. We can't rerun those test unless we un-delete the branch, though.

The "array to string conversion" and "called statically" errors under PHP 5.5 are also known, and require building a PHP 5.5 testing environment to fix. INN/deploy-tools#41

PHP 5.5 is used by WPEngine and PHP 5.3 is now 19 months past end of life.

@aschweigert aschweigert merged commit 6a5cfc6 into develop Apr 12, 2016
@aschweigert aschweigert deleted the 934-two-column-hero branch April 12, 2016 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants