Prevent position reset when props change #28
Conversation
Add a new `resetOnPropsChange` props to prevent breaking changes to existing code
|
Can you add a test for this? |
Co-authored-by: Colin Tinney <[email protected]>
Co-authored-by: semantic-release-bot <[email protected]>
|
@cdtinney I have added the test. Sorry for the late response... |
|
No problem - can you change this to merge into |
| @@ -23,6 +23,17 @@ describe('Marquee', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('does not reset the position when resetOnPropsChange is set to false on prop updates', (done) => { | |||
There was a problem hiding this comment.
Can we add a test for when resetOnPropsChange is true, even though it's the default? i.e. mock the timer, make the text scroll halfway across, then change the props?
We can probably use something like https://github.com/alexreardon/raf-stub
There was a problem hiding this comment.
It is possible to do animation in jest? inner.clientWidth seems to always be 0 which make the position never correct.
The only thing I can thinks of is compare the current _pos.x to the _getInitialPosition() when props changed, which again, not going to work when the position is incorrect.
I have went through the library you suggested, it seems like a library that help us doing stuff between every raf, handy though, but again, we can't do anything without correct position.
I'm not good in jest actually, if you do know how to do it correctly and have time to do so, simply close this pr and do you own changes, don't worry about me haha. But I am willing to help.
There was a problem hiding this comment.
Ahh, sorry. It's been a while since I've worked on this. I thought I used requestAnimationFrame originally.
In this case, I think we'd be best served by an integration test that visually compares the position, e.g. checking the x-axis of the text itself after props change. This would basically mimick the visual test you're doing when you made the change.
We could add a data-test-inner-text attribute to the inner div, and check the relative position of that?
There was a problem hiding this comment.
I'm going to pull this branch tonight and see if I can figure a test out.
cdtinney
force-pushed
the
develop
branch
2 times, most recently
from
7830188 to
100f30a
Compare
Fixes #14
Currently, position will reset when props is being updated.
Added a new
resetOnPropsChangeprops to prevent breaking changes to existing code.