Skip to content

Conversation

@nandorojo
Copy link
Contributor

@nandorojo nandorojo commented Feb 23, 2021

Added support for an optional validRange prop. Closes #48.

Usage:

<DatePickerModal 
  validRange={{
    startDate: new Date(),
    endDate: dateInTheFuture()  
  }}
/>

Both startDate and endDate are optional.

List

  • Disable clicking items not matching range
  • Blur items outside of the range
  • Make range opt-in only, per-field
  • Add validRange to example app

@nandorojo nandorojo mentioned this pull request Feb 23, 2021
@RichardLindhout
Copy link
Member

RichardLindhout commented Feb 24, 2021

Hey looks good,

I have some feedback but I could be wrong:
Instead of converting to iso and to date back again to prevent re-rendering the useCallback please use the useLatest hook

@nandorojo
Copy link
Contributor Author

I turned the dates into strings so that the same date would maintain the same reference to useCallback across renders. However, if someone updates the actual time for the start or end of the range, then the function should probably receive a new reference altogether.

I figured this was the desired outcome, since useCallback also has other variables in its dependency array which will cause it to create a new reference.

That said, if someone uses new Date() directly in render code, it could cause the code to constantly update on every render. So I think your feedback makes sensee.

@RichardLindhout
Copy link
Member

Yeah I know, but that's exactly what the useLatest hooks solve since it will not re-render since the dependency is a ref!
I like that you think of perfomance since it's indeed slow if the useCallback get re-rendered, but the useLatest hook should solve that issue :)

@nandorojo
Copy link
Contributor Author

nandorojo commented Feb 24, 2021

Yeah, makes sense. I wanted to make sure we avoid stale values, but I think that should work fine.

@RichardLindhout
Copy link
Member

RichardLindhout commented Feb 24, 2021

Yeah should work good https://twitter.com/dan_abramov/status/1063801478584897536. I use useLatest for more things so if it does not work the library has a problem anyway because I use the same trick in other places to prevent re-rendering

@nandorojo
Copy link
Contributor Author

nandorojo commented Feb 24, 2021

Fixed.

I left it as a string for src/Date/Month, since this memoization is used in render code, not onPress. If we use a ref here, the UI won't update if the range dates change.

@nandorojo
Copy link
Contributor Author

Also included exports to close #47

@nandorojo
Copy link
Contributor Author

nandorojo commented Feb 25, 2021

@RichardLindhout just confirming that sounds good to you. We should be triggering a re-render if the datetime string changes. Otherwise, changing the valid range would result in stale UI.

In the case of a press, using a ref is fine.

@nandorojo
Copy link
Contributor Author

I'm using a fork now in my app, it's all working well.

@RichardLindhout RichardLindhout merged commit b135125 into web-ridge:master Mar 10, 2021
@nandorojo nandorojo deleted the valid-range branch March 10, 2021 20:04
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.

Disable/blur old dates

2 participants