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 and redo history #617

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Kitenite
Copy link
Contributor

@Kitenite Kitenite commented Mar 31, 2024

Summary

This is a POC for undo redo. Only working for colors but is simple to add for any edit types. Adapting changes from my own internal repo so please excuse some poor formatting.

Testing

I added some unit test but you can also test this by applying a background change then CMD+Z to undo and CMD+SHIFT+Z to redo.

Description

The idea is to store reversible events for any edits, then have an applyEvent call that can call the reverse event (akin to the command pattern).

Possible improvements

One possible improvement to this is to save the old style as a parameter in the element. This way, the old style is always ensured to be the original style. This requires some refactoring of the way styles are applied in Visbug but is how we do it in my project.

let oldStyles = element.dataset.oldStyles
        ? JSON.parse(element.dataset.oldStyles)
        : {};

      // Save the current style value to the map before updating, only if it doesn't exist
      if (oldStyles[key] === undefined) {
        const oldStyle = element.style[key];
        if (oldStyle !== undefined) {
          // Save only if there's a current value
          oldStyles[key] = oldStyle;
          element.dataset.oldStyles = JSON.stringify(oldStyles); // Serialize and save back to dataset
        }
      }

      // Update the style
      element.style[key] = value;
      // Emit event
      handleEditEvent({
        el: element,
        editType: EditType.STYLE,
        newValue: { [key]: value },
        oldValue: { [key]: oldStyles[key] },
      });

Copy link

google-cla bot commented Mar 31, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@argyleink
Copy link
Member

very exciting, thank you for sharing 🙂

there's a lot of your work here that crosses over how my mind had architected it. I hadnt considered deduping tho or debouncing, nice ideas! also, the tests are great, thanks

could the pattern handle adding and removing elements? i mean, i bet it could be taught to, but in the current state it looks to deal a lot with nodes and not a node's participation in a tree. was just curious.

how often is that unique selector used instead of the node as the key?

i imagine margin/padding/font/position adjustments are pretty easy to do next?

small favor, consider temporarily disabling your formatter before a PR, so it's easier for me to see which lines you actually changed 😅

@Kitenite
Copy link
Contributor Author

Kitenite commented Apr 1, 2024

could the pattern handle adding and removing elements? i mean, i bet it could be taught to, but in the current state it looks to deal a lot with nodes and not a node's participation in a tree. was just curious.

Yes, this can be extended to inserting, removing and reordering components by storing the parent, index in the parent and the selector itself. This would also work with text content as well. Using something like the Component below. I'm actively working on coming up with a good schema for this atm, I would appreciate your thoughts on this.

export type EditEvent = {
  createdAt: string;
  selector: string;
  editType: EditType;
  newVal: Record<string, string> | Component | TextVal;
  oldVal: Record<string, string> | Component | TextVal;
}

export type TextVal = {
  text: string;
}

export enum EditType {
  TEXT = "TEXT",
  STYLE = "STYLE",
  ATTR = "ATTR",
  INSERT = "INSERT",
  REMOVE = "REMOVE",
}

export interface Component {
  selector: string;
  parentSelector: string; // Parent selector
  index: number; // Index within parent
  content: string; // String content of the element (For reversibility)
}

how often is that unique selector used instead of the node as the key?

I like the selector better since it can be stored across sessions or exported as a string (to reproduce on another browser for example). But a weakmap to the element on the same session works too.

i imagine margin/padding/font/position adjustments are pretty easy to do next?

Positions are trickier, more on the insert-remove side but the rest are pretty straight forward.

small favor, consider temporarily disabling your formatter before a PR, so it's easier for me to see which lines you actually changed 😅

Yeah sorry I noticed that happening after making the PR but had already messed up everything 😞. Will do next time.

Let me know if you think this is worth pursuing and I can find some time to clean up and add more :)

@argyleink
Copy link
Member

def worth pursuing 🙂 this would be a very valuable feature, worthy of the effort. happy to help you think things out and be an early adopter while it evolves.

@Kitenite
Copy link
Contributor Author

Kitenite commented Apr 3, 2024

Sounds good, I'll get to cleaning it up

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.

2 participants