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

Remove duplicate hero image on posts, update largo_byline on older templates #127

Merged
merged 4 commits into from
Nov 14, 2019

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Nov 14, 2019

Changes

This pull request makes the following changes:

Why

For #124

Testing/Questions

Features that this PR affects:

  • Article content

Questions that need to be answered before merging:

  • Is this PR targeting the correct branch in this repository?
  • does it work?

Steps to test this PR:

  1. Check it out.

@benlk benlk requested a review from joshdarby November 14, 2019 17:33
@benlk benlk self-assigned this Nov 14, 2019
@joshdarby
Copy link

joshdarby commented Nov 14, 2019

@benlk I'm confused (probably because I don't have any background knowledge on the way they've been using featured media like you do), but wasn't the idea to use a built in Largo filter that would remove the image in the post if it matched the featured image, instead of just removing the largo_hero all together?

@benlk
Copy link
Collaborator Author

benlk commented Nov 14, 2019

The filter is presently removed from effect in

function citylimits_remove_largo_remove_hero() {
remove_filter( 'the_content', 'largo_remove_hero', 1 );
}
add_action( 'init', 'citylimits_remove_largo_remove_hero' );
, but I'm still seeing the media credits output upon the page. I'm going to download the relevant assets to see if the reason the filter is failing is because the image tags aren't being output due to lack of image.

@benlk
Copy link
Collaborator Author

benlk commented Nov 14, 2019

tl;dr: I have discovered today that the filter doesn't work on Gutenberg content. I'm writing up the rest of the problem in an issue in Largo. It boils down to the assumptions we made about those images' markup no longer being correct, in a https://stackoverflow.com/a/1732454 sort of way.

Largo issue: WPBuddy/largo#1834

@benlk
Copy link
Collaborator Author

benlk commented Nov 14, 2019

@joshdarby With WPBuddy/largo#1834 being the case, this PR's removal of largo_hero():

  • works (hopefully)
  • is maintainable

benlk added a commit that referenced this pull request Nov 14, 2019
An alternate conclusion to #127 and #124
@joshdarby
Copy link

@benlk I'm fine with it, but we may want to write out a more in-depth tldr for @MirandaEcho and @kaylima so there's no confusion about images moving forward.

@benlk
Copy link
Collaborator Author

benlk commented Nov 14, 2019

I don't think there's actually going to be any change for City Limits going forward; this is just making sure that the new article template continues the behavior of the past article templates.

@benlk benlk merged commit 40e6e0f into master Nov 14, 2019
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.

2 participants