Overview

Create a passing test that sends the expected payload that includes a change to content in a global region and that it gets saved in autosave storage.

PageTemplate and Page/Node get separate autosave records despite client sending a single tree in the payload.

Proposed resolution

User interface changes

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

larowlan created an issue. See original summary.

larowlan’s picture

Title: Implement auto-save of the page template config entity » [PP-1] Implement auto-save of the page template config entity
Status: Active » Postponed
Related issues: +#3492669: Use PageTemplate status flag to enable per-theme page templates
wim leers’s picture

Title: [PP-1] Implement auto-save of the page template config entity » Implement auto-save of the page template config entity
Status: Postponed » Active
larowlan’s picture

Title: Implement auto-save of the page template config entity » [PP-1] Implement auto-save of the page template config entity
Status: Active » Postponed
Parent issue: » #3489899: Add support for global regions

longwave made their first commit to this issue’s fork.

longwave’s picture

This has #3489899: Add support for global regions merged in and will make more sense once that has landed in 0.x.

At present autosave and reload of page templates seems to work, next steps is to actually render the autosaved page template, plus the saved model needs adjusting to extract only the parts relevant to the content entity and page template entity instead of saving the entire thing.

wim leers’s picture

Title: [PP-1] Implement auto-save of the page template config entity » Implement auto-save of the page template config entity
Status: Postponed » Needs work
longwave’s picture

Rebased following #3489899: Add support for global regions being merged.

longwave’s picture

I've tied this in with #3492669: Use PageTemplate status flag to enable per-theme page templates and ensured that:

  • If the page template is enabled, we send all regions to the layout and preview endpoints
  • If the page template is disabled or doesn't exist, we only send the content region
wim leers’s picture

Assigned: larowlan » longwave
Priority: Normal » Major

#10++ — nice! 😄

Many questions on the MR, but this is definitely going in the right direction! 👍

Assigning to @longwave because he's actively working on it, and Lee is hopefully asleep 😊

larowlan’s picture

Status: Needs work » Needs review
longwave’s picture

Assigned: longwave » wim leers

Assigning to Wim for review, I've reviewed this and think that we are in good shape with it, and can't see anything left to do except @todos that are already tagged.

I do think that we could try and remove the root UUID now as it's a bit meaningless, we do special steps to ensure it exists, but why does it have to exist at all? Opened #3495625: Remove ComponentTreeItemList::ROOT_UUID from hydration and client-to-server conversion to investigate that.

effulgentsia’s picture

This looks fantastic, but an E2E test is now failing. Possibly due to a recent commit to 0.x?

effulgentsia’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Fixed

I might be wrong, but I think Wim is off work this whole week for the holidays, so I reviewed this in his stead, and it looks great to me, so I merged it. @wim leers: I hope I didn't miss anything important that you would have caught.

wim leers’s picture

Assigned: Unassigned » larowlan
Status: Fixed » Patch (to be ported)
Issue tags: +Needs screenshots, +Needs followup
Related issues: +#3489772: [PP-1] Add a param converter and DTO for XB data model
StatusFileSize
new2.41 MB

@effulgentsia This MR had many more commits after my December 19 review than before. This is one of the bigger MRs merged into XB! So to remain on top of this project, I had to review it in its entirety, to understand all changes.

Manual test

CI is passing, but I can't get this to work … 🙈 — when I manually test this, I get this without the Use Experience Builder for page templates in this theme. checkbox at /admin/appearance/settings/oliverochecked: — a reinstall fixed it, even though it was a fresh install. 👍 Not sure what happened there.

At minimum, this needs a screenshot illustrating what is newly possible, preferably a screencast. Is the attached GIF a correct illustration?

If it is accurate (please remove the Needs screenshots tag then!), then there's a clear bug present: placed blocks that do not render anything in the current context (most obvious one: the "Messages" block) take up vertical space, making the entire preview look off 😅 Tagging Needs followup for that.

Either way: SO COOL to see this starting to work! 😄😎

Code review

I did do a partial review (ran out of time) of the MR, and would like you to confirm 2 statements to be confirmed to ensure I understand it correctly — and one detail in PageTemplateDisplayVariant I'm not sure about. 🙏

larowlan’s picture

Issue tags: -Needs screenshots
StatusFileSize
new28.1 MB

Here's a screen recording that was shared in community slack during a recent meeting.

I'll add a follow up for the spacing

larowlan’s picture

Status: Patch (to be ported) » Fixed
Issue tags: -Needs followup

Added #3497744: If an autosave entry exists before enabling global regions for a theme, theme regions cannot be seen which I experienced whilst getting screenshots for the spacing follow up. Perhaps that explains the issue you saw in manual testing Wim?

Opened #3497747: Global regions containing only "dynamically missing blocks" (due to emptiness or access) cause malformed pages for the whitespace - its coming from CSS in Olivero targetting the .region class

wim leers’s picture

Assigned: larowlan » Unassigned

#19: yes, #3497744: If an autosave entry exists before enabling global regions for a theme, theme regions cannot be seen sounds like a highly probable explanation! Thanks for #3497747: Global regions containing only "dynamically missing blocks" (due to emptiness or access) cause malformed pages, too. Bumped both to Critical.

This allowed me to update #3455753 in a detailed way for the status of the 19. Modify the page template deliverable: #3455753-32: Milestone 0.2.0: Early preview.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.