Skip to content

Conversation

@IzzleNizzle
Copy link
Contributor

Great package here.

I was wondering if you think this would be a good implementation of a Reset function.

I like this package and need a reset functionality and would love to see this implemented.

I tried to avoid any breaking changes. The only change is to the useTimer function. It now returns an additional parameter which is the reset function.

You then send the reset function a second offset as a parameter.

I have shown the functionality in the App.js file, I added a new button there to see it in action.

I also changed the Start button to use the subtractSecond function rather than calculateExpiryDate as I felt this is a more consistent fix.

Please let me know what you think!

@amrlabib
Copy link
Owner

amrlabib commented Apr 12, 2019

Thanks a lot @IzzleNizzle happy to know the package is helping :) ... Also thank you for the PR

I Checked the PR and have few things that we can discuss together.

  1. I think we should pass to restart function newTimestamp instead of passing offset just to be consistent with what we originally pass to useTimer hook, so we will just need to do that calculation of timestamp from outside the hook the same way we did it when we passed initial expiryTimestamp, this way the initial value and the value passed in restart will be in the same format. what do you think ?

  2. Changing calculateExpiryDate to subtractSeconds for consistency, i agree with you lets do that

  3. I think updating expiryTimestamp settings from inside useTimer is not the correct way working with hooks, because based on my understanding useTimer is getting called with each render and React is doing the magic for keeping the state of each useTimer function call, this means when you updated expiryTimestamp it got updated in one function call only but for React the value is still the old one.

Because of that, now useTimer will not handle the following case correctly resart -> pause -> start if you test this scenario the timer will pause and then start again but with the old expiryTimestamp not the new one that we set while calling restart.

So i think we can try changing expiryTimestamp to be state, and then update it with restart, i tried something like the below and seems to work, what do you think ?

This is just the new added part, the rest of useTimer will remain the same

  const { expiryTimestamp: expiry, onExpire } = settings || {};
  const [expiryTimestamp, setExpiryTimestamp] = useState(expiry);
  .
  .
  .

  function restart(newExpiryTimestamp) {
   reset();
   setExpiryTimestamp(newExpiryTimestamp);
  }
  .
  .
  .  
  // didMount effect
  useEffect(() => {
    start();
    return reset;
  }, [expiryTimestamp]);

@IzzleNizzle
Copy link
Contributor Author

Thanks for your fast response! This has been pretty fun to work on with you!

In regards to those points you discussed, I'll explain my thoughts and actions below:

  1. I agree with you and so I updated the restart function to accept a newExpiryTimestamp parameter which needs to be calculated outside of this package. This aligns with the pattern that the userTimer function uses, so this is more unified.

  2. Glad you agree, change implemented and no further comment.

  3. I've made the changes to the expiryTimestamp state variable like you suggested. I think it's a good solution.

I hope these changes are good. I think it's a good feature!

@amrlabib amrlabib merged commit 06e62fc into amrlabib:master Apr 16, 2019
@amrlabib
Copy link
Owner

Thanks you very much @IzzleNizzle for adding restart functionality.

restart is now available in v1.1.5

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