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

[Feature] Map Editor Undo should be stored in localStorage #3

Open
cpojer opened this issue May 7, 2024 · 6 comments · May be fixed by #22
Open

[Feature] Map Editor Undo should be stored in localStorage #3

cpojer opened this issue May 7, 2024 · 6 comments · May be fixed by #22
Labels

Comments

@cpojer
Copy link
Contributor

cpojer commented May 7, 2024

Right now, when you accidentally reload you lose undo/redo state. Ideally undo/redo state is stored in an encoded form (MapData.toJSON) in localStorage and is therefore persisted after a reload, ensuring that players do not accidentally lose a map in progress. See the MapEditor component. It's also worth checking out the updateUndoStack function.

I suggest storing the undo state by mapObject?.id (or a fallback id if no mapObject is provided) so the undo stack can be per-map which will be on a different route in the game.

Additionally, when loading the map editor it has the ability to "restore" previous state, like from the last playtest. It should no longer be necessary to keep that feature separate, and instead the code should be cleaned up so that restorePreviousState just restores from the undo stack. This might need a fallback behavior so that it doesn't keep restoring the same previous map – or alternatively the "Restore Map" button in the MapEditorControlPanel should just be an "Undo" button after the first click (or after undo/redo is executed once). Open to ideas to ensure a good user experience.

Links

Funding

  • We're using Polar.sh to distribute funds.
  • You receive the reward once the issue is completed & confirmed by Nakazawa Tech.
Fund with Polar
@cpojer cpojer added the good first issue Good for newcomers label May 7, 2024
@cpojer cpojer changed the title [Feature] Map Editor undo should be stored in localStorage [Feature] Map Editor Undo should be stored in localStorage May 7, 2024
@cpojer cpojer added the editor label May 8, 2024
@polar-sh polar-sh bot added the Fund label May 12, 2024
@connorlindsey
Copy link
Contributor

I'd be interested in taking a pass at this one after the share url issue is complete since it touches similar parts of the code unless someone else wants to before then.

@cpojer
Copy link
Contributor Author

cpojer commented May 16, 2024

Go for it!

@tjamesmac
Copy link

Oh I was beaten to the punch! I have been working on this a bit so probably my fault for not mentioning it sooner, is it okay if I keep plugging away @connorlindsey?

I'm at the point where i've got the undo stack restoring on reload and restoring the previous map. But I have a few questions about the MapObject.id and the restore previous state UX.

@connorlindsey
Copy link
Contributor

@tjamesmac Yeah, go ahead 😄

@tjamesmac
Copy link

Thank you!

My questions were:

  • The MapObject.id is returning undefined and never returns an actual id. Is this expected? If it is, do you have any suggestions for generating a decent fallback?

  • Instead of restoring the previous map with the Restore button, i'm restoring if a previousState is present. When clicking New map I then delete the related localstorage items so a refresh doesn't continually bring back the last map. Does this sound like the right path for the U? Happy to change if I misinterpreted the requirement.

  • Also not sure if i'm missing something obvious but the confirmation dialogs for the new/reset map don't seem to work. I've just started calling new map directly

  • Lastly, are there tests for the map editor?

@tjamesmac tjamesmac linked a pull request May 17, 2024 that will close this issue
2 tasks
@cpojer
Copy link
Contributor Author

cpojer commented May 17, 2024

Hey @tjamesmac, thanks for working on this and sorry for the late reply, I was at a conference. Let's take it to the PR you created.

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

Successfully merging a pull request may close this issue.

3 participants