Closed (fixed)
Project:
Experience Builder
Component:
Page builder
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
28 May 2024 at 09:03 UTC
Updated:
20 Jun 2024 at 18:09 UTC
Jump to comment: Most recent
Problem/Motivation
We're using redux to manage state
This makes redo/undo support reasonably straight forward
Steps to reproduce
Proposed resolution
Consider redux-undo package or implement our own per https://redux.js.org/usage/implementing-undo-history
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork experience_builder-3450303
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:
- 3450303-undo-redo
changes, plain diff MR !36 - sidepanels-undo changes, plain diff MR !28
Comments
Comment #2
larowlanComment #4
hooroomooComment #5
jessebaker commentedAs mentioned in the description, this is hopefully fairly easy to implement through Redux. I met with hooroomoo and we discussed that both the layout and model slices of the redux state will need to be added to the undo "array" so that changes to both the layout and the props/details of a component will be tracked as undo-able states.
Comment #7
wim leersGreat to see this moving! 🤩
I suspect end-to-end tests using Cypress is overkill for this. (Happy to be wrong.) Unit tests seem appropriate for such a foundational capability?
Comment #8
hooroomooWill continue off 3450303-undo-redo now that sidepanels branch is merged
Comment #12
hooroomooCreated #3452781: Add tests for undo/redo to write tests. Would like to get this in first since it has a good amount of refactoring.
Comment #13
hooroomooComment #14
wim leersComment #15
wim leersBoth @larowlan and @jessebaker have approved the MR.
I think that means this is RTBC?
But shouldn't we have (unit) tests prior to merging?
Comment #16
wim leersAh, @jessebaker pointed out that for tests, we have the follow-up #3452781: Add tests for undo/redo, which is blocked on another issue. Great!
Comment #17
wim leersNeeds Ben's MR approval before it can be merged 👍
Comment #18
hooroomoolets goooo