Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(date-input): use hooks #963

Merged
merged 5 commits into from
Jul 29, 2019
Merged

refactor(date-input): use hooks #963

merged 5 commits into from
Jul 29, 2019

Conversation

montezume
Copy link
Contributor

Summary

Refactors the DateInput component to use hooks.

inputRef.current.setSelectionRange(0, 100);
emit(date);
},
[inputRef]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this makes sense 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my understanding yes. useCallback does not have smart exceptions like any value of props or any ref. It is just a closure and memoization as far as I understood it.

this.props.value === '' ? null : getDateInMonth(this.props.value) - 1,
};

showPrevMonth = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was never used

const nextDate = changeMonth(calendarDate, amount);
setCalendarDate(nextDate);
setHighlightedIndex(0);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason to not useCallback these two?

Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks.

@emmenko emmenko force-pushed the ml-hooks-date-input branch from ef38b1a to 6cefdef Compare July 29, 2019 08:15
calendarDate: today,
highlightedIndex:
prevState.suggestedItems.length + getDateInMonth(today) - 1,
const { onChange } = props;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is how I would go about it too. Thanks.

},
});
};
}, [onBlur, props.id, props.name]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here

@emmenko emmenko merged commit 938b518 into master Jul 29, 2019
@emmenko emmenko deleted the ml-hooks-date-input branch July 29, 2019 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants