Extend Undo/Redo functionality to LO #8819
Conversation
src/app/utils/undo-redo-history.ts
Outdated
| // Needed for type checking, TS otherwise seems to get lost | ||
| // in weaker overloads of `useReducer`? | ||
| const reducer: typeof historyReducer<S> = historyReducer; | ||
| const [{ state, undoStack, redoStack }, dispatch] = useReducer( | ||
| reducer, | ||
| initialState, | ||
| initializer | ||
| ); |
There was a problem hiding this comment.
I did not expect this to be valid syntax, and neither does VS Code's syntax highlighting
there's gotta be a better way to do it but I couldn't find it
There was a problem hiding this comment.
Hm yeah that's unfortunate. I'm not sure why it has trouble with that typing.
There was a problem hiding this comment.
So I did some research and while we probably can't fix type inference/useReducer overloads, the solution I've used here is an application of TS 4.7 instantiation expressions. What I had tried first was useReducer(historyReducer<S>, ...), which ought to work according to that feature's documentation, but there are bugs in VS Code that make constructs like these cause spurious errors:
microsoft/TypeScript#50191
microsoft/TypeScript#50161
It looks like the typeof workaround throws off the parser enough for it to not issue these kinds of semantic errors 🤷
robojumper
force-pushed
the
lo-history
branch
from
651c039
to
26392f8
Compare
|
|
||
| > * { | ||
| flex: 1; | ||
| margin-right: 4px; |
There was a problem hiding this comment.
We have a couple helpers for this sort of thing - @include horizontal-space-children(4px), or @include flex-gap(4px) if you want to handle cases where the buttons wrap.
| | { type: 'undo' } | ||
| | { type: 'redo' }; | ||
|
|
||
| function lbUIReducer(state: LoadoutBuilderUI, action: LoadoutBuilderUIAction) { |
There was a problem hiding this comment.
Neat, I like having these be split up.
A la Redux, if you have the reducers do default: return state, you can just call all of them for all actions and maybe simplify your combined reducer. But it's not that big a deal here.
| ...lbUIState, | ||
| }, | ||
| dispatch, | ||
| { canUndo, canRedo }, |
There was a problem hiding this comment.
Any reason to break these out like this instead of returning them in the big state blob?
src/app/utils/undo-redo-history.ts
Outdated
| // Needed for type checking, TS otherwise seems to get lost | ||
| // in weaker overloads of `useReducer`? | ||
| const reducer: typeof historyReducer<S> = historyReducer; | ||
| const [{ state, undoStack, redoStack }, dispatch] = useReducer( | ||
| reducer, | ||
| initialState, | ||
| initializer | ||
| ); |
There was a problem hiding this comment.
Hm yeah that's unfortunate. I'm not sure why it has trouble with that typing.
robojumper
force-pushed
the
lo-history
branch
from
26392f8
to
6a5ab46
Compare
robojumper
force-pushed
the
lo-history
branch
from
6a5ab46
to
2e96cea
Compare
Splits LO into configuration state and UI state, wraps all config actions in a History, and commits like seven different React crimes to make it transparent to existing code.