-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Import existing undo/redo history when initializing the Undoo instance #69
Conversation
…e, if any history is provided
…mplate to avoid code duplication
@amcdnl PR ready, please review ;) |
…uldn't reflect on the component
…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
Made some improvements/changes. Ready to be merged IMHO. |
maxLength: maxHistory | ||
}) | ||
); | ||
const undoo = new Undoo({ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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.');
});
I fixed the small stuff, need your help regarding the undoo manager ref stuff. |
@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) |
@Vadorequest - Given the age, I'm going to close this PR. If its still something relevant let's reopen and start discussions. |
@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. |
Push up tests and lets itterate on other |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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
asUndoProps
to preload an initial history when initializing theundoo
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 theuseUndo
hook. 🎉And added Storybook story for useUndo examples.
Does this PR introduce a breaking change?
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.