- Issue created by @lauriii
- Assigned to hooroomoo
- First commit to issue fork.
- Merge request !99#3460952: Implement add section and add element buttons → (Merged) created by hooroomoo
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@hooroomoo raised terminology/functionality questions in a meeting yesterday wrt "sections". I continued that over at #3455036-13: [research] Clarify "components" vs "elements" vs "patterns" → , to make sure we formally define these so that it'll be clearer for the next contributor.
- 🇺🇸United States hooroomoo
This work is built on top of 📌 Implement component states Fixed 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. 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.
- 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.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.
- Tests
- Update issue summary to not deal with slots - Issue was unassigned.
- Status changed to Needs work
about 2 months ago 8:10am 18 July 2024 - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- Assigned to Utkarsh_33
- First commit to issue fork.
In the latest commit i was able to get the following things done that were remaining from #6 📌 Implement add button for top level item (section) Fixed :-
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.Now the button is more inline with the design.Attaching the screenshots for the same.
- 🇫🇮Finland lauriii Finland
The "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.
- First commit to issue fork.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@jessebaker: Is it okay for @Utkarsh_33 to continue here?
- 🇬🇧United Kingdom jessebaker
@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 📌 DDEV support for Cypress tests Fixed is complete and tests are easier to write I think some tests to ensure the following should be implemented
- Ensure that after selected a component at the root level, a button is shown that says "Add section". Clicking that button should open the menu to the Section Templates part of the left menu.
- Ensure that after selected a component below the root level (e.g. a component inside a slot), a button is shown that says "Add component". Clicking that button should open the menu to the Components part of the left menu.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Thanks, those are very clear instructions! 👍😊
- 🇺🇸United States hooroomoo
This should be rebased again now that 📌 Implement component states Fixed which this MR was originally built on top of is in HEAD :)
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@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... 😊
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Clearly, @Utkarsh_33 has already started writing tests prior to 📌 DDEV support for Cypress tests Fixed being ready, so removing the prefix 🚀
I still think that one #3458369: DDEV support for Cypress tests is in then it would be easier for me to resolve the test failures.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
I tried reverting the changes to
a11y.cy.js
andxb-general.cy.js
, because I was 99% convinced that those changes were unnecessary, and were causing the tests to fail.I was wrong! 😅
- Issue was unassigned.
- Status changed to Needs review
about 1 month ago 12:30pm 29 July 2024 Addressed major bunch of feedbacks.Just a small question left then it's again up for a review.
- Assigned to hooroomoo
- Status changed to Active
about 1 month ago 3:32pm 30 July 2024 - Status changed to Needs review
about 1 month ago 1:38pm 31 July 2024 - Issue was unassigned.
- Assigned to bnjmnm
- Status changed to Needs work
about 1 month ago 5:34pm 31 July 2024 - 🇺🇸United States bnjmnm Ann Arbor, MI
The 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)
- Assigned to hooroomoo
- Assigned to bnjmnm
- Status changed to Needs review
about 1 month ago 9:30pm 31 July 2024 - 🇺🇸United States hooroomoo
#36
The 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)
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.
- 🇺🇸United States bnjmnm Ann Arbor, MI
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.
It should be possible to use an intercept in Cypress to have a test that uses the
layout-default.json
file instead of the backend data. If that isn't possible, explaining why in a comment will be helpful. - Assigned to hooroomoo
- Status changed to Active
about 1 month ago 1:49pm 1 August 2024 - Assigned to bnjmnm
- Status changed to Needs review
about 1 month ago 9:40pm 2 August 2024 - 🇺🇸United States hooroomoo
It should be possible to use an intercept in Cypress to have a test that uses the layout-default.json file instead of the backend data. If that isn't possible, explaining why in a comment will be helpful.
It 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). Below is a photo of an artifact from the test run. 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.
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.
- Assigned to jessebaker
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
to add a test when we work on supporting nested components.
Ohh! 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?
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
👆 AFAICT this means we need a new issue for the front end, because this means the acceptance criteria for 📌 Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed 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 component
be accurate?@jessebaker is back from vacation today, I'll meet with him in a bit, so I'll ask him to start here :)
-
hooroomoo →
committed e86c7d86 on 0.x
Issue #3460952 by hooroomoo, Utkarsh_33, jessebaker, Wim Leers, bnjmnm:...
-
hooroomoo →
committed e86c7d86 on 0.x
- Issue was unassigned.
- Status changed to Fixed
about 1 month ago 2:21pm 6 August 2024 - 🇺🇸United States 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
Automatically closed - issue fixed for 2 weeks with no activity.