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

Fix layout issue with Academia theme #2773

Merged
merged 4 commits into from Oct 15, 2019

Conversation

@alexsanford
Copy link
Contributor

alexsanford commented Aug 19, 2019

Fixes #2771

The Problem

The Academia theme, as per the issue linked above, seemed to have problems with our theme compatibility code. This stemmed from the fact that when we use a "dummy post" for rendering, we give it an ID of 0. Something about this made the theme behave unexpectedly.

I traced it through to the fetching of metadata, and I'm thinking it is related to this code, which returns false instead of an empty string when the object ID is 0. Also, the theme itself has some conditional logic that checks specifically for an empty string, so when get_metadata returns false, it will behave differently than when the meta does not exist (see #).

The Solution

The solution I'm attempting here is to set the "dummy post" ID to a valid, non-zero number. That said, I do not want to set it to a number that is being used as a valid ID in the database, because I feel like that could cause other problems. Instead, I fetch the largest post ID currently in the database and add 1. This query should only be run once per request (I think?) so the performance penalty should be negligible.

Testing Instructions

  • See #2771 to test on Academia.
  • We should test archive pages on several themes to make sure that the ID change doesn't cause some other unexpected behaviour.
Copy link
Contributor

pgk left a comment

Works as advertised -- No broken layout on the archive page. I see some failing tests around the post id being nonzero, once those are amended, I think :shipit:

@alexsanford

This comment has been minimized.

Copy link
Contributor Author

alexsanford commented Aug 20, 2019

Got the tests passing and fixed an issue exposed by another test (explicit check for ID of 0).

On the surface, I think this one is ready to go!

That said, let's hold off until the next release so we can get some more 👀 on it, and more testing 🙂

Copy link
Collaborator

donnapep left a comment

Tested on a bunch of themes and it looks good to me!

@jom
jom approved these changes Oct 15, 2019
Copy link
Member

jom left a comment

Looks good to me!

@donnapep donnapep merged commit e32087b into master Oct 15, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@donnapep donnapep deleted the fix/theme-compatibility-theme-id branch Oct 15, 2019
@donnapep donnapep changed the title Set dummy post ID to the next available post ID Fix layout issue with Academia theme Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.