Custom scroll in year picker and allow editing button #202
Conversation
…dent-scroll feat: add custom scroll and allow editing button
|
@TVGSOFT looks good but we let it slip, could you fix the merge conflict on this so we could merge? |
|
I have resolved the conflict, please help to review and merge this PR. Thank a lot! |
merge resolve duplicated import
jcloquell
left a comment
jcloquell
left a comment
There was a problem hiding this comment.
Hi there! I know this was already merged (and also released in the 0.14.0 version), but I found these issues that I wanted to share with you, in case you want to address them :)
| locale={locale} | ||
| editIcon={props?.editIcon} | ||
| calendarIcon={props.calendarIcon} | ||
| allowEditing={props.allowEditing || true} |
There was a problem hiding this comment.
This is always true. Shouldn't it be {props.allowEditing ?? true} instead?
There was a problem hiding this comment.
Hi @jcloquell Yup I agree.
it should be allowEditing={props.allowEditing ?? true}
| if (inputDate !== undefined) { | ||
| allowEditing = inputDate | ||
| } | ||
| const isAllowEditing = allowEditing && mode !== 'multiple' && inputDate !== undefined |
There was a problem hiding this comment.
I think this change brought some undesired side effects, as
if (inputDate !== undefined) {
allowEditing = inputDate
}
is not equivalent to making const isAllowEditing = ... && inputDate !== undefined.
The first is overriding the value of isAllowEditing with inputDate if this is defined.
The second is having an impact on the isAllowEditing value depending on inputDate being defined or not.
At the same time, the existing inputDate property is and was never set from anywhere outside where this component is used, so its value is always undefined.
All of this make the edit button to always be hidden regardless the value passed for the allowEditing prop.
There was a problem hiding this comment.
Hi @jcloquell, thanks for catching this! Would you mind creating a quick PR to address this?
There was a problem hiding this comment.
@jcloquell i think can change to this:
const isAllowEditing = allowEditing && mode !== 'multiple'
And remove this:
if (inputDate !== undefined) { allowEditing = inputDate }
What do you think?
5 participants
Enhance some features:
Fixed bug from #201