Make WordPress Core

Opened 6 weeks ago

Closed 5 weeks ago

#59770 closed defect (bug) (fixed)

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

Reported by: onemaggie's profile onemaggie Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.4
Component: Bundled Theme Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

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

Added a new block pattern for the home template that inherits the page query and fits the design of the home page.
Minor labeling issues
Fixed parameter for wp_get_theme()
Fixes categories for some patterns

Props:

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

Change History (30)

This ticket was mentioned in PR #5593 on WordPress/wordpress-develop by @onemaggie.


6 weeks ago
#1

  • Keywords has-patch added

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

  • Added a new block pattern for the home template that inherits the page query and fits the design of the home page.
  • Minor labeling issues
  • Fixed parameter for wp_get_theme()
  • Fixes categories for some patterns

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

#2 @onemaggie
6 weeks ago

  • Keywords has-patch removed

#3 @kebbet
6 weeks ago

If this is for 6.4, please milestone the ticket accordingly.

#4 @hellofromTonya
6 weeks ago

  • Keywords has-patch changes-requested added
  • Milestone changed from Awaiting Review to 6.4
  • Version set to trunk

Pulling into the 6.4 milestone as this ticket will be included in 6.4 RC3.

There are changes requested in the patch with a PR opened upstream in the TT4 repo. Will wait to review for commit until its resolved.

#5 @rajinsharwar
6 weeks ago

This is the PR reported in the TT4 upstream: https://github.com/WordPress/twentytwentyfour/pull/712

After that PR is reviewed, I guess this is good to be marked as 'commit', as https://github.com/WordPress/wordpress-develop/pull/5593 already resolves the issue with the blog query being ignored and breaking the navigation.

#6 @hellofromTonya
6 weeks ago

  • Keywords changes-requested removed
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

PR 712 is now merged into the Core patch. Removing changes-requested and beginning commit review.

#7 @hellofromTonya
6 weeks ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested:https://github.com/WordPress/wordpress-develop/pull/5593

Environment

  • OS: macOS
  • Web Server: wp-env
  • PHP: 7.4
  • WordPress: trunk
  • Browser: Firefox
  • Theme: Twenty Twenty-Four
  • Active Plugins: none

Test static Blog page

Test instructions

Followed the instructions here from #59759 here. Copying here for completeness:

To replicate (use TT4 theme in its default configuration):

  1. Ensure pretty permalinks are enabled
  2. Ensure there are at least 5 posts on your website (content doesn't matter).
  3. On Settings > Reading, configure "Your homepage displays" to a static page (choose any page) and also set a "Posts page" (e.g. to an empty page called "Blog" with /blog/ slug).
  4. Set "Blog pages show at most" to 4.
  5. Navigate to /blog/. 🐞 You'll immediately notice that this page looks like a home page...
  6. Scroll down to where it says "Watch, Read, Listen". You'll see the 3 latest posts, even though your site is configured to show 4 posts per page (which should be the case here since your site has 5+ posts).
  7. Navigate to /blog/page/2/.
  8. Scroll down to "Watch, Read, Listen" again. You'll still see the 3 latest posts, even though you would expect the next set of posts to be displayed on this URL.

Actual Results

  • Before the patch, can produce #59759's reported issue.
  • After the patch, the /blog/ static page renders as expected, i.e. resolved the issue ✅

Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.

#8 @hellofromTonya
6 weeks ago

In testing, also noticed:

Scenario:

  1. Create a new page called Home and use the "Business home" pattern.
  2. Navigate to Settings > Reading.
  3. In the "Your homepage displays" section, set "A static page (select below)" and select the "Home" page as the "Homepage".
  4. View the frontend and scroll down to the "Watch, Read, Listen" section.

Notice no blog posts render. Instead only the Home page (i.e. the static page itself) renders in that section.

This behavior happens before and after applying the patch.

Was the expectation this section would render the blog posts with this latest patch?
Is this known issue?

@onemaggie

@hellofromTonya commented on PR #5593:


6 weeks ago
#9

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

#10 @onemaggie
6 weeks ago

@hellofromTonya it feels like it should work, the query is inheriting from the page. The theme is not doing anything to prevent that from happening so that might be a different bug happening there.

@richtabor commented on PR #5593:


6 weeks ago
#11

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.

#12 @onemaggie
6 weeks ago

@hellofromTonya when I set inherit to false, I get the full list of posts, if I set it to true, only the current page shows!

@hellofromTonya commented on PR #5593:


6 weeks ago
#13

This is why I leaned towards using front-page.html instead of home.html https://github.com/WordPress/twentytwentyfour/pull/706#issuecomment-1785404020.

@felixarntz added his thoughts https://github.com/WordPress/twentytwentyfour/pull/706#issuecomment-1785447074.

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

#14 @richtabor
6 weeks ago

Scroll down to where it says "Watch, Read, Listen". You'll see the 3 latest posts, even though your site is configured to show 4 posts per page (which should be the case here since your site has 5+ posts).

I'm seeing the posts render properly within the query loop on /page/2/, showing "Hello world!" and "Test 1" (I have Test 1-5 published as well as Hello world).

But it is strange to have the existing homepage now the blog page.

@richtabor commented on PR #5593:


6 weeks ago
#15

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.

#16 @hellofromTonya
6 weeks ago

@richtabor @onemaggie @flixos90 for the Home static page issue, please advise if PR 5593 should or shouldn't be committed yet.

A follow-up commit could happen if there's consensus on what to do with the Home static page issue.

Please advise.

@onemaggie commented on PR #5593:


6 weeks ago
#17

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

@flixos90 commented on PR #5593:


6 weeks ago
#18

@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. 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.

@onemaggie commented on PR #5593:


6 weeks ago
#19

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 commented on PR #5593:


6 weeks ago
#20

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.

#21 @hellofromTonya
6 weeks ago

  • Keywords commit added

Patch: https://github.com/WordPress/wordpress-develop/pull/5593

Marking PR 5593 for commit to trunk.

@richtabor commented on PR #5593:


6 weeks ago
#22

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.

#23 @hellofromTonya
6 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 57036:

Twenty Twenty-Four: Bugfixes for 6.4 RC2.

This update includes the following bugfixes:

  • Fix: Added a new block pattern for the home template that inherits the page query and fits the design of the home page. (more context on the theme repo: https://github.com/WordPress/twentytwentyfour/pull/706)
  • Fix: Rely on parent theme data for block style.
  • Fix: Categories for some patterns.
  • Fix: Minor labeling issues

Follow-up to [56999], [56951], [56813], [56764], [56716].

Props anlino, beafialho, desrosj, devmuhib, didierjm, fabiorubioglio, flixos90, hanneslsm, hellofromTonya, huzaifaalmesbah, ktaron, luminuu, mshowes, onemaggie, phillsav, poena, rajinsharwar, richtabor, shailu25.
Fixes #59770, #59759.

Last edited 5 weeks ago by hellofromTonya (previous) (diff)

#24 @hellofromTonya
6 weeks ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 2nd committer sign-off to backport [57036] to 6.4 branch.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


6 weeks ago

@hellofromTonya commented on PR #5593:


6 weeks ago
#26

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.

#27 @flixos90
5 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

Thanks for the commit @hellofromTonya! LGTM for backport.

#28 @jorbin
5 weeks ago

After a brief discussion in slack, LGTM as well.

#29 @hellofromTonya
5 weeks ago

Thanks to you both @flixos90 and @jorbin. Preparing the backport commit now.

#30 @hellofromTonya
5 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57037:

Twenty Twenty-Four: Bugfixes for 6.4 RC3.

This update includes the following bugfixes:

  • Fix: Added a new block pattern for the home template that inherits the page query and fits the design of the home page. (more context on the theme repo: https://github.com/WordPress/twentytwentyfour/pull/706)
  • Fix: Rely on parent theme data for block style.
  • Fix: Categories for some patterns.
  • Fix: Minor labeling issues

Follow-up to [56999], [56951], [56813], [56764], [56716].

Reviewed by flixos90, jorbin.
Merges [57036] to the 6.4 branch.

Props anlino, beafialho, desrosj, devmuhib, didierjm, fabiorubioglio, flixos90, hanneslsm, hellofromTonya, huzaifaalmesbah, ktaron, luminuu, mshowes, onemaggie, phillsav, poena, rajinsharwar, richtabor, shailu25.
Fixes #59770, #59759.

Note: See TracTickets for help on using tickets.