-
-
Notifications
You must be signed in to change notification settings - Fork 143
Fix Min/Max DatePickerRange date #561
Conversation
Also added example usage to Demo. Added isRequired to id props, since the downstream component has them required anyway
|
Thanks for the fix @mmartinsky! I'd like to see if we can avoid The solution that comes to mind is making a uuid for each one and storing that in We've used |
|
The previous commit passed all 3 python ones, not sure how I can retrigger builds - can you please take a look @alexcjohnson ? |
|
@mmartinsky I wouldn't worry about those test failures - the python one is an intermittent error we know about (@byronz you were talking about |
generate a uniqid from uniqid package on construction if not supplied
remove formatting
| startDateId={start_date_id} | ||
| endDateId={end_date_id} | ||
| startDateId={this.state.start_date_id} | ||
| endDateId={this.state.end_date_id} |
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 think we need to do props.start_date_id || this.state.start_date_id here rather than in the constructor - just in case a user decides to change props.start_date_id later on. No idea why someone would do that, but if they did it would not make it to the <DateRangePicker> this way.
|
Hi I tried to fix the issue, shouldn't this be i tried a lot but this was the only things i found |
|
Also i found something else that might be helpful shouldn't this be I still don't know how can i test it , so please could you help |
|
@Vishalghyv Yeah, that's the crux of this PR - it was mistakenly looking at state vs. props in https://github.com/plotly/dash-core-components/pull/561/files#diff-63268a7882b62c59b51c626fe11e9ca1R77. If you want to test it, you can clone my fork and run |
|
@mmartinsky That means i did found the crux of the problem :) :))))) |
alexcjohnson
left a comment
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.
Beautiful. Thanks so much for the contribution & iteration @mmartinsky! 💃
Fixes #551, #557
Also added example usage to Demo.
Added isRequired to id props, since the downstream
component has them required anyway