Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented Apr 12, 2017

This pull request seeks to introduce an undo history for blocks state.

undo

Implementation notes:

The undo implementation is roughly based on the one explored in official Redux documentation.

redux-undo is a popular alternative module which exists to implement this behavior, but also includes a number of features we don't need.

To the previous point, the changes include a proposed combineUndoableReducers which defines getters for transparent property access on the original reducer object (allow access by state.blocks.byUid directly rather than state.blocks.present.byUid). I feel ever-so-slightly uncomfortable including enhanced objects like this in state.

We may want to move these utilities to a separate file or, more likely, a separate module, but they're included here for initial discussion before being referenced more formally.

Testing instructions:

Verify that unit tests pass:

npm run test-unit

Confirm that upon changing a text block†, the undo buttons become activated in the editor header and behave as expected upon toggling.

† The text block's change behavior is currently only triggered when blurring the contenteditable, meaning you'll need to click elsewhere on the screen first before the buttons become active.

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 12, 2017
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally satisfied with this implementation for now. We could revisit later.

editor/state.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally fine with this

editor/state.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the same simplification in #407 (We'll have some conflicts)

editor/state.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's time to start splitting those reducers into separate files

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep in mind the cognitive overload nd perceived complexity for people whenever we split files. (Part of why redux is perceived as more complex than what it is.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's time to start splitting those reducers into separate files

Is this feeling compounded by the addition of the utilities to the file? I'd like to have those moved first before anything else.

editor/state.js Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the thinking behind this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the thinking behind this?

Memory, but in hindsight it might be a bit premature. We can remove it.

@aduth
Copy link
Member Author

aduth commented Apr 13, 2017

  • Rebased to resolve conflicts
  • Removed options.limit from undoable
  • Moved reducer enhancer to utils/undoable-reducer.js

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Later, we'll probably need to keep track of the focus config in the state. This would allow the undo/redo to move the cursor to the right position

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Framework Issues related to broader framework topics, especially as it relates to javascript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants