Overview

Pages should be created and edited in Experience Builder, not the normal Drupal entity form

Proposed resolution

See #3482259: Landing page integration: new content entity type for unstructured content.

Set up link handler and link templates that match XB experience_builder.experience_builder route

User interface changes

Issue fork experience_builder-3487075

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:

About issue forks

Comments

mglaman created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Component: Data model » Page
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Adding or editing a Page brings user into Experience Builder not entity form » [PP-1] Adding or editing a Page brings user into Experience Builder not entity form
Issue summary: View changes
Status: Active » Postponed
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Title: [PP-1] Adding or editing a Page brings user into Experience Builder not entity form » Adding or editing a Page brings user into Experience Builder not entity form
Issue summary: View changes
Status: Postponed » Active
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Assigned: Unassigned » mglaman
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Found a flaw. The route experience_builder.experience_builder requires a saved entity. There isn't a "new entity" route yet. I don't know if this issue should be modifying \Drupal\experience_builder\Controller\ExperienceBuilderController::__invoke to allow $entity to be nullable.

'base' => \sprintf('xb/%s/%s', $entity->getEntityTypeId(), $entity->id()),

This would break if null. But the entity type is a route parameter, so it could be added as a method argument.

Then in the method, if the entity is null we could pass an ID of `0`?

mglaman opened merge request !416

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Active » Needs work
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

+1'd your first proposal, as did @lauriii, so I think you're unblocked 😄

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Assigned: mglaman » Unassigned
Status: Needs work » Needs review

Ready for some full reviews!

wim leers credited f.mazeikis.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » mglaman
Status: Needs review » Needs work

Looking good!

Asked for a bunch of clarifications, and I think I see a whole range of small simplifications. 😇

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review

Needs review. I think we can update some @todo to point to #3489775: Consider removing baseQuery in favour of explicity requiring the params in each api call.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

🏓 @mglaman, see #3489302-39: Preview entire page not just content area WRT the blocker.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: mglaman » wim leers
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » mglaman
Status: Needs review » Needs work

empty-canvas.cy.js has been failing since https://git.drupalcode.org/issue/experience_builder-3487075/-/pipelines/.... I suspect it's related to the big E2E test refactor in #3481736: Adapt E2E tests to work with auto-save. So I just reverted that file to origin/0.x's and re-wrapped it in the .forEach that @mglaman did. And … I arrived at the exact same set of changes, not even a single character difference.

Others have touched the E2E tests more often than I have and are better equipped (and have more time) to debug this.

See review on the MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... for the other bits of feedback.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Assigned: mglaman » Unassigned
Status: Needs work » Needs review

Replied about the enhancer. And the test passed once I added video recording to debug... so let's see if it passes again when I remove videos.

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

hooroomoo opened merge request !443

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Pushed the one clean-up commit I apparently failed to push on Dec 3 🙈

There's only 2 hunks that I cannot approve in principle:

  1. one in the E2E test infra, but it's a trivial change: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... — and confirmed by others in another MR
  2. a fairly trivial update to ui/tests/e2e/empty-canvas.cy.js, which was also worked on by @hooroomoo, who's done their fair share of writing/expanding E2E tests

So, bypassing approval for those 2 small hunks only.

  • wim leers committed 7ec187a7 on 0.x authored by mglaman 
    Issue #3487075 by mglaman, wim leers, hooroomoo, lauriii, f.mazeikis:...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

wim leers closed merge request !443

Status: Fixed » Closed (fixed)

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