- Issue created by @f.mazeikis
- ๐ณ๐ฑNetherlands balintbrews Amsterdam, NL
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. - @fmazeikis opened merge request.
- ๐ซ๐ฎFinland lauriii Finland
Parent issue is already tracked as a stable blocker.
- First commit to issue fork.
- ๐ฌ๐งUnited Kingdom f.mazeikis Brighton
FE folk need this in order to start FE part of Folder work, so I've extracted unfinished tasks into #3541364 ๐ Expand Folder entity functionality Active and moving this into review.
- ๐ณ๐ฑNetherlands balintbrews Amsterdam, NL
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. Maybeparent
? Any other ideas? - ๐ณ๐ฑNetherlands balintbrews Amsterdam, NL
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 toitems
. - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
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 componentsxb_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
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
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
- 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 ๐ )
- The issue summary should be updated to point the bullets delayed to ๐ Expand Folder entity functionality Active to #3541364. ๐
- This is a new config entity type in XB, hence it needs its own new section in
docs/config-management.md
- Various smaller (e.g. test coverage, methods with a single test user โฆ) and bigger (not using config entities' native
uuid
property) on the MR.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Nice progress, the BE MR is getting close to ready! ๐
- ๐ฌ๐งUnited Kingdom f.mazeikis Brighton
Still need to add documentation, will have a go at it first thing tomorrow. The rest can be reviewed.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
- Was able to simplify the "ID + UUID and must be kept in sync" situation: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- Guaranteed a consistent config export order: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- 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...
- Missing update path test coverage: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- 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! ๐
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐จ 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 oneFolder
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โข๏ธ? ๐ - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Update path test looks good! ๐
Remaining:
- #20.5 (
status
) - docs (
docs/config-management.md
) - checking item uniqueness (discussed with Felix in private chat โ we realized that it's currently possible for one item to appear in multiple folders ๐ )
- #20.5 (
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Everything in #22 has been addressed!
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Two last remarks:
- I don't think
::loadByItemAndType()
should return an array https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... - One bit of missing test coverage in
testOneFolderPerItemLimitConstraintValidation()
: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... - Nit: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- I don't think
-
wim leers โ
committed e5c8abcf on 1.x authored by
f.mazeikis โ
Issue #3539729 by f.mazeikis, wim leers, balintbrews: Implement Folder...
-
wim leers โ
committed e5c8abcf on 1.x authored by
f.mazeikis โ