Closed (fixed)
Project:
Experience Builder
Component:
Page builder
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
12 Jul 2024 at 08:37 UTC
Updated:
20 Aug 2024 at 14:24 UTC
Jump to comment: Most recent, Most recent file



Comments
Comment #2
lauriiiComment #5
wim leers@hooroomoo raised terminology/functionality questions in a meeting yesterday wrt "sections". I continued that over at #3455036-13: Clarify "components" vs "elements" vs "patterns", to make sure we formally define these so that it'll be clearer for the next contributor.
Comment #6
hooroomooThis work is built on top of #3460783: Implement component states so those commits are in the MR as well. The logic and architecture is basically done other than some details that need to be worked out (e.g. first bullet in Still left to do section below). The logic is if a node is on the root level, than the Add Section button renders. If the node has a parent, then the Add component button renders.
MR in its current state
- All primary menu related actions now live in its own slice called primaryMenuSlice.ts
- Introduces a new ClickToInsertState in the slice.
- The logic to handle where to insert a node based on if it is a component vs. section lives in clickToInsertHandler in List.tsx
- AddButton component dispatches necessary info to the primaryMenuSlice which is used by the clickToInsertHandler.
- Replaced Submenu with another Menubar.Root so we can control opening the inner menu in redux.
- Added a first border to the first list item when a user clicks the 'Add *' button to encourage clicking
Still left to do:
- Decide on behavior after user has inserts a new node. Should the user be able to insert multiple nodes consecutively or should the menu close?
- Fix styling regression on second level menu where hovered
- General styling for the new 'Add Component/Section' button.
- Check it works with undo/redo
- Tests
- Update issue summary to not deal with slots
Comment #7
hooroomooADD SECTION

ADD COMPONENT

Comment #8
wim leersThanks for those GIFs in #7 🤩 Adding them to the issue summary to encourage somebody else to continue where @hooroomoo left things (thanks for the clear write-up about the current state in #6 too! 👏), because @hooroomoo is out for a few days 🏝️
Comment #9
lauriiiAssigning to @Utkarsh_33! 😊
Comment #10
wim leersWelcome, @Utkarsh_33! 😄🥳
Comment #12
utkarsh_33 commentedIn the latest commit i was able to get the following things done that were remaining from #6:-
1)Close the sidebar menu once the user selects section/component.
2)Fixed styling regression on second level menu where hovered.
3)Centred the Add section/component button.
Still left to do::-
- Check it works with undo/redo
- Tests
- Update issue summary to not deal with slots.
Comment #13
lauriiiHmm, this doesn't look same as the design?
Comment #14
utkarsh_33 commentedNow the button is more inline with the design.Attaching the screenshots for the same.
Comment #15
utkarsh_33 commentedFinal state:-
Comment #16
lauriiiThe "Add section" button should be only active when the component is active / selected, meaning that it should not be visible when hovering over a component that is not currently active / selected. Currently the button appears in the hover state, even when the component is not active.
Comment #18
wim leers@jessebaker: Is it okay for @Utkarsh_33 to continue here?
Comment #19
jessebaker commented@Wim Leers
Yes this can be continued. I recommend that when @hooroomoo is back tomorrow that they and @Utkarsh_33 sync to ensure there is a clear decision as to who continues this work to push it over the finish line.
Once #3458369 is complete and tests are easier to write I think some tests to ensure the following should be implemented
Comment #20
wim leersThanks, those are very clear instructions! 👍😊
Comment #21
hooroomooThis should be rebased again now that #3460783: Implement component states which this MR was originally built on top of is in HEAD :)
Comment #22
wim leers@Utkarsh_33 FYI: see what @hooroomoo wrote in #21, which I explained in some more detail at https://git.drupalcode.org/project/experience_builder/-/merge_requests/9... 😊
Comment #23
wim leersClearly, @Utkarsh_33 has already started writing tests prior to #3458369: DDEV support for Cypress tests being ready, so removing the prefix 🚀
Comment #24
utkarsh_33 commentedI still think that once #3458369: DDEV support for Cypress tests is in then it would be easier for me to resolve the test failures.
Comment #25
utkarsh_33 commentedFixed AddSection test!
Comment #26
wim leersI tried reverting the changes to
a11y.cy.jsandxb-general.cy.js, because I was 99% convinced that those changes were unnecessary, and were causing the tests to fail.I was wrong! 😅
Comment #27
utkarsh_33 commentedI will fix those.On my way!
Comment #28
utkarsh_33 commentedComment #29
utkarsh_33 commentedAddressed major bunch of feedbacks.Just a small question left then it's again up for a review.
Comment #30
hooroomooComment #31
hooroomooComment #32
hooroomooComment #33
hooroomooComment #34
hooroomooComment #35
wim leersComment #36
bnjmnmThe issue summary mentions both "add section" and "add component" (for slots). I do not see the latter here - that should either be addressed int he MR or given its own issue (the additional issue might be more manageable IMO)
Comment #37
hooroomooComment #38
hooroomoo#36
Add component button is supported on the frontend side. But currently we don't have testable nested component/slots yet so there's no way to test it besides with the hard-coded layout-default.json file.
Comment #39
bnjmnmIt should be possible to use an intercept in Cypress to have a test that uses the
layout-default.jsonfile instead of the backend data. If that isn't possible, explaining why in a comment will be helpful.Comment #40
hooroomooComment #41
hooroomooIt was possible to have an e2e test to use our mock json files. However, our front-end code throws many console errors when dealing with nested components (which makes sense because we haven't had to support it yet). I looked into it and the app gets confused of what is the currently selected component when it hovers over a nested component so sometimes the selectedElement is null which results in an error being thrown. The value eventually gets set so it passes locally and when doing it manually but in the CI, it decides to crap out whenever there is an error. Below is a screenshot artifact from a CI run.
Since supporting nested components is out of scope for this issue, I updated the IS and added a todo in add-section.cy to add a test when we work on supporting nested components.
Comment #42
wim leersOhh! I thought the UI/front end was ahead of the back end, in that it did support nested components already, I'd swear I'd seen that in a brief demo you did, @hooroomoo? 😅 Or perhaps that just happened to be a subset of the UI functionality for which enough pieces were in place already, i.e. maybe it's just the component hovering/selecting that does not yet support nested components?
👆 AFAICT this means we need a new issue for the front end, because this means the acceptance criteria for #3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" are now vaguer: I thought the UI was sufficiently far ahead to be able to observe the entire UX, but knowing this, I think having that only preview a component tree is sufficient, @tedbow should not expect to be able to selector or edit it.
Would a new issue titled
Add support for selecting a nested componentbe accurate?@jessebaker is back from vacation today, I'll meet with him in a bit, so I'll ask him to start here :)
Comment #44
hooroomoo#42:
I almost want to wait until the backend can support/provide the frontend with a nested component before fixing the console issues from hovering over a nested component so we don't make changes based off of hard-coded JSON file where the structure might become outdated