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.

Sign up for GitHub

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

Jump to bottom

Only future date optional prop added to DatePicker #203

Closed
wants to merge 1 commit into from
Closed

Only future date optional prop added to DatePicker #203

wants to merge 1 commit into from

Conversation

adamalexandru4
Copy link

#173
Added new optional constraint for minimum date allowing only for future date or not.

@vercel
Copy link

vercel bot commented Sep 30, 2020 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/code4romania/taskforce-fe-components/8e94fjzfb
✅ Preview: https://taskforce-fe-components-git-fork-adamalexandru4-date-picker.code4romania.vercel.app

surdu
surdu requested changes Oct 1, 2020
View reviewed changes
src/components/form/datePicker.js
@@ -57,7 +60,7 @@ export const DatePicker = ({
);
};

const DateOnlyPicker = ({ startDate, maxDate, onChange }) => {
const DateOnlyPicker = ({ startDate, maxDate, onChange, onlyFutureTime }) => {
Copy link
Member

@surdu surdu Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't be more flexible to have it as a minDate prop instead ?

Copy link
Author

Choose a reason for hiding this comment

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

I used another field due to requirements. @aniri said that it should be a property set to true / false.
minDate should be a Date not a Bool by definition.

Copy link
Member

Choose a reason for hiding this comment

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

I believe that she meant the property in the question entry. This shouldn't be to different from what is already implemented for allowFuture in the previous link.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for my mistake,. I will rework the code tonight.

Copy link
Member

Choose a reason for hiding this comment

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

yes, @surdu is right, sorry for the confusion. as I mentioned in #173 , the implementation should be similar to the one for allowFuture

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surdu surdu surdu requested changes

@aniri aniri Awaiting requested review from aniri aniri is a code owner

@catileptic catileptic Awaiting requested review from catileptic catileptic is a code owner

@moonflare moonflare Awaiting requested review from moonflare

@RaduCStefanescu RaduCStefanescu Awaiting requested review from RaduCStefanescu

@RazvanCiuca RazvanCiuca Awaiting requested review from RazvanCiuca RazvanCiuca is a code owner

@tudoramariei tudoramariei Awaiting requested review from tudoramariei tudoramariei is a code owner

@Utwo Utwo Awaiting requested review from Utwo Utwo is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamalexandru4 @aniri @surdu