Overview

Follow up to #3491459: Implement the "Review N changes" button

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.

effulgentsia’s picture

Besides changing this endpoint, is there any other complication we can foresee with breaking the 1:1 relationship between what the UI shows in the pending changes list and what corresponds to an entity? Hopefully, it's no big deal, but just wanting to ask before we go down that road.

larowlan’s picture

@effulgentsia

Rough implementation plan:

  • (ideally) Get #3500087: Make AutoSaveManager::getAutoSaveKey static in
  • Add a new method to autosave manager saveGlobalRegion - this just appends the region name to the existing key and label
  • Change ApiPreviewController to store one entry per model region instead of in one hunk by calling this new method
  • Update ApiPublishAllController to group page template entries in $all_auto_saves and join them before calling the existing code

So whilst the title says 'pending changes api' the actual changes are elsewhere.

larowlan’s picture

Assigned: Unassigned » larowlan

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

longwave’s picture

Status: Active » Needs review
wim leers’s picture

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs review » Active
Related issues: -#3500087: Make AutoSaveManager::getAutoSaveKey static
larowlan’s picture

larowlan’s picture

Status: Active » Needs review
wim leers’s picture

Assigned: Unassigned » longwave
Related issues: +#3489899: Add support for global regions

Per https://git.drupalcode.org/project/experience_builder/-/merge_requests/5..., I'd like @longwave's thoughts here in particular because it's thanks to him we have docs/adr/0005-Keep-the-front-end-simple.md, and @larowlan's remark reveals that here we're deviating from that.

longwave’s picture

Assigned: longwave » Unassigned
effulgentsia’s picture

longwave’s picture

Just thinking out loud a bit, but are we over complicating by trying to split the config entity both here and eventually in Workspaces? We created the config entity - should page_template be a page_region config entity instead? This would simplify all the cases we have so far - perhaps there are downsides I haven't thought of.

larowlan’s picture

I think it's worth exploring that idea. I'm not sure if this is the issue for that or we should postpone this on a spike

wim leers’s picture

Title: The pending changes API endpoint should list individual regions for global template changes » [PP-1] The pending changes API endpoint should list individual regions for global template changes
Status: Needs review » Postponed

Per #3501600-7: Split 1 PageTemplate config entity into N PageRegion config entities, I now think it's quite likely we should not do this, and do that instead. Postponing until #3501600 lands and we're 100% confident we won't need (something like) this.

@longwave: well predicted! I see at least one nice bonus benefit — see bottom of that comment 😄

effulgentsia’s picture

Issue tags: +sprint
wim leers’s picture

99% certain that we're going with #3501600 per #3501600-13: Split 1 PageTemplate config entity into N PageRegion config entities, just needs @lauriii confirmation for me to tackle this next week 👍

wim leers’s picture

Title: [PP-1] The pending changes API endpoint should list individual regions for global template changes » The pending changes API endpoint should list individual regions for global template changes
Status: Postponed » Active
wim leers’s picture

Status: Active » Closed (outdated)
Related issues:

we want to only show the regions that actually have changed.

That is indeed the remaining challenge after #3501600: Split 1 PageTemplate config entity into N PageRegion config entities:

But … this is a problem already for the content entity's XB field already. For that, we have #3502902: Only auto-save content entities/PageRegion config entities when there are actual changes: simply previewing incorrectly causes them to appear in "Review x changes". Both the edited content entity and the global regions (1 PageTemplate with N component trees, and as of minutes ago, N PageRegion config entities with 1 component tree each) get their auto-save states created through \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable(), so it's all already in a single place already anyway.

Hence my proposal to tackle that all in #3501600: Split 1 PageTemplate config entity into N PageRegion config entities.

nagwani’s picture

Issue tags: -sprint