Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add undo/redo to the builder #486

Merged
merged 8 commits into from
Apr 21, 2024
Merged

Conversation

legnes
Copy link
Contributor

@legnes legnes commented Apr 16, 2024

This PR adds undo/redo to the builder. It captures and persists the entire grid state during a number of actions, including clicking a fill suggestion and cut/paste.

Some questions I'm curious to hear others' thoughts on:

  • Should there be menu items for undo/redo?
  • Are there other parts of the state that need to be captured?
  • Are there other features that could benefit from some kind of auto-side-effect or reducer middleware logic? I went with a manual approach for now, but if there's need for a larger composable system to watch and modify state changes, I could take a crack at it!

Closes #14

app/reducers/reducer.ts Outdated Show resolved Hide resolved
app/lib/types.ts Outdated Show resolved Hide resolved
app/reducers/reducer.ts Outdated Show resolved Hide resolved
app/reducers/reducer.ts Outdated Show resolved Hide resolved
@mdirolf
Copy link
Member

mdirolf commented Apr 17, 2024

Looks great (and seems to work great locally, too). There was one minor fix needed for z vs Z when parsing the input. I'm about 50/50 on whether to add an item to the more menu - the main plus i think would be to make folks aware that this feature exists.

On mobile I think the best we can support is via the more menu but I'm not sure if that'd be ergonomic enough to even be useful. I can imagine some potential ergonomic improvements on mobile as followup issues (i.e. maybe after the first More > Undo press the builder goes into "undo mode" and the top bar displays an undo and redo button until a non-history action is taken) but I think we should definitely wait on those.

As for other parts of the state to capture - I think this seems correct to me? Can't think of anything else we should add.

As for other reducer middlewares I can't think of anything there either but (as we've discussed offline) I'm definitely open to any improvements or refactorings to the reducer setup. I do think this commit does a great job of being surgical about adding history.

@legnes
Copy link
Contributor Author

legnes commented Apr 20, 2024

Pushed some updates here @mdirolf

@mdirolf mdirolf merged commit 9ee1902 into crosshare-org:master Apr 21, 2024
1 check passed
@mdirolf
Copy link
Member

mdirolf commented Apr 21, 2024

Thank you!

@legnes
Copy link
Contributor Author

legnes commented Apr 21, 2024

Hey @mdirolf, sorry to ask you to take another look.

As I was rebasing, I realized that this was missing some coverage for rebuses -- there are a lot of ways to cancel out of rebus editing that don't restore the previous cell contents, so the old approach probably would've needed a pushToUpdate() for every time closeRebus() was called.

On the other hand it occurred to me that as long as the update stack is diffing anyway for deduplication, there's no reason to be tracking individual changes. After every action, just before the builder reducer exits, it can check if the grid has changed and if so push to the stack. I guess it's sort of a less generic version of the middleware idea we were talking about!

Since this now runs every action, I thought about trying to reduce the overhead by adding a sort of version number to the grid state that would get incremented each edit, and that could be used to speed up diffing. But when I tested things out with the max allowable grid, the deep diff was taking pretty much no time (like 0-1ms), so unless the grid state bulks out appreciably I think this should be okay as-is!

@legnes
Copy link
Contributor Author

legnes commented Apr 21, 2024

Oh hey you merged it hahaha 🤦

@legnes legnes deleted the undo-redo branch April 21, 2024 12:04
@mdirolf
Copy link
Member

mdirolf commented Apr 21, 2024

I think that change to running it on every action seems to make the code pretty clean, too, which is a nice bonus! Looks great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undo/Redo in constructor for clicking on fill suggestions
2 participants