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

Twenty Twenty-Four: bugfixes and changed pattern for posts on home template #5593

Closed
wants to merge 3 commits into from

Conversation

MaggieCabrera
Copy link

@MaggieCabrera MaggieCabrera commented Oct 30, 2023

This PR includes the bugfixes from the GH repo since RC 2.

Trac ticket:
https://core.trac.wordpress.org/ticket/59770

Props:

@onemaggie
@luminuu
@beafialho
@poena
@flixos90
@richtabor
@huzaifaalmesbah
@devmuhib
@mshowes
@ktaron
@hanneslsm
@Shailu25
@anlino
@phillsav
@didierjm
@fabiorubioglio
@desrosj


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @MaggieCabrera! This looks good to commit in terms of the fix from WordPress/twentytwentyfour#706. I have two questions on the other two changes.

Copy link
Member

@felixarntz felixarntz 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 the updates, LGTM!

Copy link
Member

@richtabor richtabor left a comment

Choose a reason for hiding this comment

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

LGTM

@hellofromtonya
Copy link
Contributor

Noted an issue with a static Home page here. Cross-posting for awareness.

@richtabor
Copy link
Member

Noted an issue with a static Home page here. Cross-posting for awareness.

This is why I leaned towards using front-page.html instead of home.html here.

@felixarntz added his thoughts here.

@hellofromtonya
Copy link
Contributor

This is why I leaned towards using front-page.html instead of home.html WordPress/twentytwentyfour#706 (comment).

@felixarntz added his thoughts WordPress/twentytwentyfour#706 (comment).

Can this be renamed as part of this Core resync patch?

@richtabor
Copy link
Member

Can this be renamed as part of this Core resync patch?

I'm curious of @felixarntz's objections, or any others. It seems the sensible approach perhaps, but I know there are a lot of if/thens to consider.

@MaggieCabrera
Copy link
Author

Isn't it a core/editor bug that the query block needs to be set to inherit:false in this specific use case? I don't see how renaming the home to front-page solves this particular issue

@felixarntz
Copy link
Member

felixarntz commented Oct 31, 2023

@hellofromtonya @richtabor Unfortunately, renaming home.html to front-page.html wouldn't be a solution to the problem outlined in https://core.trac.wordpress.org/ticket/59770#comment:8. Here's why:

  • When the "Your homepage displays" setting is set to "A static page", it currently uses the regular page.html template. So whatever you put into the actual page will be applied. If you use a template that renders the current posts loop within a page, you'll see the loop containing only that page.
  • Doing so doesn't make sense, but apparently is possible. This is technically expected behavior, but it should probably prevented in the UI somehow (not in scope of this PR).
  • Renaming home.html to front-page.html would actually make this problem worse, because then the static page would use front-page.html instead of page.html. So you would then have that experience regardless of what you put into the individual page you choose for the home page.

In other words: While the current experience is not ideal, renaming home.html to front-page.html would make it worse. We will not be able to properly address these issues without something like https://core.trac.wordpress.org/ticket/59767 and without potentially further UI considerations.

I think this PR should be merged as is, since it addresses https://core.trac.wordpress.org/ticket/59759 and several other issues. The other problems need to be continued to be explored, but they are certainly not trivial to fix. I doubt we'll get to a solution in 6.4.

@MaggieCabrera
Copy link
Author

I agree with @felixarntz and I think that this specific use case will most probably be avoided in the future if we go the route of hiding the reading settings when using block themes, in favor of using/editing templates instead (which makes much more sense for these themes)

@hellofromtonya
Copy link
Contributor

Alrighty, thank you everyone. Moving forward with the commit for this PR. The static home page issue can be dealt with separately, i.e. wherever the root cause lies.

@richtabor
Copy link
Member

Renaming home.html to front-page.html would actually make this problem worse, because then the static page would use front-page.html instead of page.html. So you would then have that experience regardless of what you put into the individual page you choose for the home page.

I don't think this makes it worse—it makes the home page work like the posts page does, where the contents of the page do not matter; only the template. The issue is just surfaced a bit higher because it's not only the posts page that's odd (which we've become used to). However, the change does depart from the existing reading settings, where any page can be the home page's post contents.

As-is (with the current trunk TT4 experience), if you change the reading settings, you loose the homepage (it becomes the standard page template with post contents) and your blog looks like the existing homepage design. That's quite a rough edge, and maybe it's not a big deal—as the reading settings are buried within the WP admin—but it's certainly unexpected.

I think this PR should be merged as is, since it addresses https://core.trac.wordpress.org/ticket/59759 and several other issues. The other problems need to be continued to be explored, but they are certainly not trivial to fix. I doubt we'll get to a solution in 6.4.

Agreed, on all accounts. I think we can figure out an elegant solution that works across the board, in 6.5 perhaps.

@hellofromtonya
Copy link
Contributor

Committed to trunk via https://core.trac.wordpress.org/changeset/57036. Waiting for 2nd committer sign-off to backport it to the 6.4 branch.

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.

4 participants