Overview

We are adding ability to group/categorise Components via Experience Builder UI.
User can create new Folders via UI. Folders are not required to contain any Components in it.
User needs to be able to drag and drop Components from one Folder to another and arrange Folders in specific order.
When deleting a Folder, any Components contained within should be disabled.

→ See: #3540576: [META] Library organization with folders

Proposed resolution

Create new "Folder" Config entity with the following functionality:

  • Folder ID/machine name should be UUID
  • Folder should have weight, name, uiLibrary, and components properties
  • Folder names should be unique within a UI library (e.g. Components, Dynamic components, or Code)
  • Folder should keep track of what Components are assigned to it via components property
  • When new SDC based Component entity is created, we should check if a Folder matching group property of such SDC exists. If it does, place Component in that Folder. If such a Folder doesn't exist, create it.
  • Users can create, edit and delete Folders via UI
  • When User deletes a Folder, we should disable (status: false) all of Component entities it "contains", or reject deleting the folder if any of the components are currently in use
  • New Config entity should strive to reuse as much of existing API infrastructure as possible

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

f.mazeikis created an issue. See original summary.

f.mazeikis’s picture

Priority: Normal » Major
f.mazeikis’s picture

Title: Implement Category config entity » Implement Folder config entity
Issue summary: View changes
balintbrews’s picture

Issue summary: View changes

I added the uiLibrary property in the issue summary which will allow us to track which part of the XB UI the folder belongs to. Note that we also need to be able to have folders under Code, but Code is not something that \Drupal\experience_builder\Entity\Component::computeUiLibrary is aware of.

balintbrews’s picture

Issue summary: View changes
balintbrews’s picture

Issue summary: View changes
balintbrews’s picture

Issue summary: View changes
balintbrews’s picture

Component: … to be triaged » Page builder
Category: Task » Feature request
Parent issue: #3499964: Component groups for primary components (theme-provided SDCs + code components) » #3540576: [META] Library organization with folders
balintbrews’s picture

Issue summary: View changes

lauriii’s picture

Issue tags: -stable blocker

Parent issue is already tracked as a stable blocker.

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

f.mazeikis’s picture

Assigned: f.mazeikis » wim leers
Status: Active » Needs review
Related issues: +#3541364: Delete folders

FE folk need this in order to start FE part of Folder work, so I've extracted unfinished tasks into #3541364 and moving this into review.

balintbrews’s picture

Assigned: wim leers » f.mazeikis
Status: Needs review » Needs work

We previously had code in the MR that ensured that folder names are unique within a UI library. Then I asked for removing it because the concept of UI libraries is going away. What I didn't consider was that we still need unique folder names for components in the library vs. internal code components (which will eventually move to a separate menu item). I'm really sorry, but we'll need to bring back some of that code. I think we shouldn't call the property uiLibrary as that concept will still go away. Maybe parent? Any other ideas?

balintbrews’s picture

Another thing just occurred to me. Yes, I'm sorry. I don't anticipate more changes, fwiw. 🙂

We need to support organizing pattern entities, too, not just components. So I recommend that we rename the components property to items.

bnjmnm’s picture

In case it's helpful for additional testsl:

I'm working on the F.E. implementation of this and added 3 test modules that create setup content

  • xb_internal_code_components creates several internal code components
  • xb_test_patterns creates several patterns (that are essentially the same but with different names)
  • xb_test_folders creates at least two folders for patterns, library components and internal code components. It depends on the modules above + xb_test_sdc
wim leers’s picture

Very nice work, this was a pleasantly solid MR to review! 😄

At the MR level, this looks near perfect, but if I zoom out to the full scope of XB, I see some concerns.

Review

  1. The issue summary should be updated to explain the revised scope, because the MR was thoroughly confusing to read given the issue summary talks ONLY about components, but the MR also supports creating folders of patterns 🙃. Apparently this pivot happened in #15. (Please think about empowering a person reviewing this who hasn't been in any of the the in-person discussions 😅)
  2. The issue summary should be updated to point the bullets delayed to #3541364: Delete folders to #3541364. 🙏
  3. This is a new config entity type in XB, hence it needs its own new section in docs/config-management.md
  4. Various smaller (e.g. test coverage, methods with a single test user …) and bigger (not using config entities' native uuid property) on the MR.
wim leers’s picture

Nice progress, the BE MR is getting close to ready! 👏

f.mazeikis’s picture

Status: Needs work » Needs review

Still need to add documentation, will have a go at it first thing tomorrow. The rest can be reviewed.

wim leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs update path tests

Review highlights:

  1. Was able to simplify the "ID + UUID and must be kept in sync" situation: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
  2. Guaranteed a consistent config export order: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
  3. Thanks to tightening test coverage, I discovered we're missing a uniqueness validator (to prevent duplication), fixed in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
  4. Missing update path test coverage: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
  5. We should be able to modify status via the HTTP API, and shouldn't have to call logic available only in PHP when testing the HTTP API: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

Over to Felix for the last 2! 😄

wim leers’s picture

🚨 Just realized something (having just finished ensuring items in a folder are unique): shouldn't we also verify that each item can only be placed in one Folder?!

It seems a usability nightmare to see the same Component appear in multiple folders in the left sidebar? 🤔 Or … perhaps … that's actually the very intent? In any case: that's currently allowed, so we should expand test coverage for that, too, arguably.

Although, that means that in order to move a Component from one Folder to another would require first deleting it from one and then adding it to another. So perhaps keeping this as-is is reasonable, and can be considered a Feature™️? 😅

wim leers’s picture

Update path test looks good! 😄

Remaining:

  1. #20.5 (status)
  2. docs (docs/config-management.md)
  3. checking item uniqueness (discussed with Felix in private chat — we realized that it's currently possible for one item to appear in multiple folders 😅)
wim leers’s picture

Assigned: f.mazeikis » wim leers
Status: Needs work » Needs review
Issue tags: -Needs documentation

Everything in #22 has been addressed!

wim leers’s picture

Assigned: wim leers » f.mazeikis
Status: Needs review » Needs work

Two last remarks:

  1. I don't think ::loadByItemAndType() should return an array https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
  2. One bit of missing test coverage in testOneFolderPerItemLimitConstraintValidation(): https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
  3. Nit: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
wim leers’s picture

Assigned: f.mazeikis » Unassigned
Status: Needs work » Reviewed & tested by the community

wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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