Make WordPress Core

Opened 6 months ago

Closed 6 months 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 months 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 months ago

  • Keywords has-patch removed

#3 @kebbet
6 months ago

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

#4 @hellofromTonya
6 months 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 months 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 months 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 months 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 months 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

#10 @onemaggie
6 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 months 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 6 months ago by hellofromTonya (previous) (diff)

#24 @hellofromTonya
6 months 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 months ago

@hellofromTonya commented on PR #5593:


6 months 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
6 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Thanks for the commit @hellofromTonya! LGTM for backport.

#28 @jorbin
6 months ago

After a brief discussion in slack, LGTM as well.

#29 @hellofromTonya
6 months ago

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

#30 @hellofromTonya
6 months 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.