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

Import existing undo/redo history when initializing the Undoo instance #69

Closed

Conversation

Vadorequest
Copy link
Contributor

@Vadorequest Vadorequest commented Feb 24, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Not possible to preload an existing history. History gets lost during page refresh and there is no way around that.

Issue Number:

What is the new behavior?

Added an option initialHistory as UndoProps to preload an initial history when initializing the undoo instance.

Allows preserving the undo/redo history (e.g: local storage) and reload it even when the history gets lost (e.g: page refresh).

Also, added tests using @testing-library for the useUndo hook. 🎉
And added Storybook story for useUndo examples.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I first refactored the Undo.stories.tsx by using a Storybook "Template" component, but I noticed it was harder to understand the documentation when doing so.
So, I changed it back in UnlyEd@72d1ddc

See storybookjs/storybook#13657 (comment) for in-depth explanation.

I don't personally like duplicating all that code, but I noticed that's what has been done so far in other stories, and I believe the point of having Storybook is to favour documentation quality and understanding rather than code reusability.

@Vadorequest Vadorequest marked this pull request as draft February 24, 2021 17:24
@Vadorequest Vadorequest marked this pull request as ready for review February 24, 2021 18:38
@Vadorequest
Copy link
Contributor Author

@amcdnl PR ready, please review ;)

…e the "Story" doesn't contain a proper code example and it's harder to understand what's the difference between stories of the same component
@Vadorequest
Copy link
Contributor Author

Made some improvements/changes. Ready to be merged IMHO.

maxLength: maxHistory
})
);
const undoo = new Undoo({
Copy link
Member

Choose a reason for hiding this comment

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

This has to be inside the ref otherwise its recreated every render

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to reuse the undoo to use undoo.import() below, how do I keep track of the undoo reference if it's inside the ref?

Would you mind fixing this? I don't really see how it can be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing the following, but it didn't pass the tests:

  const manager = useRef<Undoo>(
    new Undoo({
      maxLength: maxHistory
    })
  );

  useEffect(() => {
    if (Array.isArray(initialHistory)) {
      manager?.current?.undoo?.import(initialHistory);
    }
  }, [initialHistory, manager]);

Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need to pass a ref in the dep array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, (but then eslint complains about it), but it doesn't fix the implementation.

I don't understand how to do that, might be simpler if you take a look yourself, if you have the time.

Copy link
Member

Choose a reason for hiding this comment

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

What did the tests complain about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last test didn't pass anymore. It basically broke the feature. Here's the output:

image

It's this test, which initializes with an existing history, but "undoing" doesn't restore it anymore:

  test('should allow to load an initial history', () => {
    // Renders the component, make "screen" available
    render(
      <UndoRedoMockComponent
        initialHistory={[{ nodes: [{ id: '1' }, { id: '2' }], edges: [] }]}
      />
    );

    expect(
      screen.getByTestId('output'),
      'Should be initialised with 0 nodes and 0 edges'
    ).toHaveTextContent('There are 0 nodes and 0 edges.');

    fireEvent.click(screen.getByText('Undo'));
    expect(
      screen.getByTestId('output'),
      'Undoing should go back to the previous state of the history, which contains 2 nodes and 0 edges'
    ).toHaveTextContent('There are 2 nodes and 0 edges.');
  });

stories/Undo.stories.tsx Show resolved Hide resolved
stories/Undo.stories.tsx Outdated Show resolved Hide resolved
stories/Undo.stories.tsx Outdated Show resolved Hide resolved
src/helpers/useUndo.ts Outdated Show resolved Hide resolved
@Vadorequest
Copy link
Contributor Author

I fixed the small stuff, need your help regarding the undoo manager ref stuff.

@Vadorequest
Copy link
Contributor Author

@amcdnl Could you have a look at this?

If you can't fix it, I suppose we should extract the new feature from the test and make another PR with the tests only, so we can merge it and increase tests coverage. (and have the feature in another PR)

@amcdnl
Copy link
Member

amcdnl commented Aug 26, 2021

@Vadorequest - Given the age, I'm going to close this PR. If its still something relevant let's reopen and start discussions.

@amcdnl amcdnl closed this Aug 26, 2021
@Vadorequest
Copy link
Contributor Author

@amcdnl I think at minimum it should be broken into 2 PR, one with the tests, and one with the new feature. The tests could/should be merged right away, but I needed your help on the feature.

@amcdnl
Copy link
Member

amcdnl commented Aug 27, 2021

Push up tests and lets itterate on other

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