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

enhancement: Datepicker: Support dynamic range #375

Merged

Conversation

vibros68
Copy link
Contributor

@vibros68 vibros68 commented Nov 3, 2021

Closes #342

This diff makes the Datepicker change the range picker if the input of
the range is changed programly.
The video show the example of preview is below. In the video the max
date is increased 1 day every 5s

vokoscreen-2021-11-03_15-05-59.mp4

@vibros68
Copy link
Contributor Author

vibros68 commented Nov 3, 2021

@lukebp ready for review

Copy link
Member

@amass01 amass01 left a comment

Choose a reason for hiding this comment

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

ACK.

Left some minors.

@@ -195,7 +198,6 @@ const DatePicker = ({
},
[setLabelMonthsState, labelMonthsState]
);

Copy link
Member

Choose a reason for hiding this comment

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

re-add

@@ -40,6 +40,9 @@ const DatePicker = ({
}) => {
const { themeName } = useTheme();
const yearArr = useMemo(() => getYearArray(years), [years]);
useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

make the useEffect a named function with a meaningful name describing
what the useEffect does, i'd add a comment as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because inside the useEffect function has only 1 line of code. So I think would add comment is enough

@vibros68 vibros68 requested a review from amass01 November 4, 2021 15:41
@vibros68 vibros68 requested a review from amass01 November 5, 2021 02:34
@lukebp
Copy link
Member

lukebp commented Nov 5, 2021

Needs approval from @tiagoalvesdulce.

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

LGTM

@tiagoalvesdulce tiagoalvesdulce merged commit 22a8e40 into decred:master Nov 8, 2021
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.

Datepicker: Support dynamic range.
4 participants