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

[HOLD for payment of bonus-only 2021-12-06] VBA- disabling future dates in the date picker #5922

Closed
SofiedeVreese opened this issue Oct 18, 2021 · 25 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Weekly KSv2

Comments

@SofiedeVreese
Copy link
Contributor

Coming from #5722


Creating a new issue for disabling future dates in the date picker and assigning that to @Santhosh-Sellavel per CME @puneetlath 's comment here.

@SofiedeVreese
Copy link
Contributor Author

@Santhosh-Sellavel proposal:

Proposal

In DatePicker add a new prop maximumDate

For iOS & Android Native files we can use maximumDate prop to restrict.

For Web/Desktop:
We have a max attribute for input type date to restrict the maximum range.
Also similarly we can customize it for minimumDate also if needed additionally.

We can take advantage of these props to avoid unnecessary min & max date range validation.

Screens

Screenshot 2021-10-15 at 1 23 24 AM

Screenshot 2021-10-15 at 1 23 07 AM

@SofiedeVreese
Copy link
Contributor Author

Exported to Upwork: https://www.upwork.com/jobs/~010a6aab5d586f3145

@SofiedeVreese
Copy link
Contributor Author

Hey @Santhosh-Sellavel are you still wanting to work on this one?

@Santhosh-Sellavel
Copy link
Collaborator

Yes @SofiedeVreese assign me this one.

Applied on upwork!

@SofiedeVreese
Copy link
Contributor Author

Perfect thanks @Santhosh-Sellavel- I've just hired you in Upwork!

@Santhosh-Sellavel
Copy link
Collaborator

@puneetlath

I've have added props to restrict date selection for the picker.

Default props value are null for both min & max. So no restriction by default.

For incorporation date picker, what is the min, max date.

@SofiedeVreese
Copy link
Contributor Author

@Santhosh-Sellavel Puneet is OOO this week, I'll try and see if I can find another CME to help you out with this!

@Luke9389 would you be able to help out with Santosh' query above in Puneet's OOO?

@Luke9389
Copy link
Contributor

@Santhosh-Sellavel I don't think we have a minimum date. The maximum should be now right? The issue is specifically focused on disallowing future dates for the incorporation date.

@Santhosh-Sellavel
Copy link
Collaborator

Yes maximum should be now or earlier. Just need a confirmation.

Also no minimumDate, I'll submit a PR tomorrow!

@SofiedeVreese
Copy link
Contributor Author

Hey @Santhosh-Sellavel I'm so sorry but @Luke9389 and I realised there are a couple of double-ups and your proposal is being tackled in another PR.

To avoid dupe solutions, we're going to close this one, so no need to work on your PR. I will be paying you out for your efforts, and will close the Upwork contract upon payment.

@SofiedeVreese
Copy link
Contributor Author

Upwork job closed, payment completed.

@Santhosh-Sellavel
Copy link
Collaborator

May I know which PR, @Luke9389 or @SofiedeVreese

@Luke9389
Copy link
Contributor

Yep, it's this one #5939

It was decided here to split the issue up into two parts, but the other contributor ended up implementing both (mistakenly I'm sure). Because their PR is already in review, and the code change that overlaps with this one looks fine in that PR, we've opted to just keep it instead of undoing the changes in that PR and then merging both PRs on the same file. It's easier to manage and cleaner this way.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Oct 29, 2021

Thanks, @Luke9389

But what I am proposing is different from what has been done in #5939
Refer to my proposal in a comment here -> #5922 (comment)

The solution I proposed is to handle the restriction in the date picker component itself just by using minDate & maxDate props, so the user won't be able to pick the future date.

But why do we need the changes in PR #5939,

Even if we disable the future dates in picker since the user doesn't have to use the picker in Web or Desktop

So only the issue was split up into two parts as said here

I think there is some misunderstanding here.
If you are aware of all the above, then we are good here!

@PrashantMangukiya
Copy link
Contributor

@Santhosh-Sellavel you are right.

You are implementing to disable future date in date picker, and my implementation in PR #5939 is to validate date during form submit and not allow future date and show error message etc.

So me and @Santhosh-Sellavel solving same problem but via different implementation as decided here to split issue #5722 (comment) as both solution is need.

So solution implemented in PR #5939 is different than @Santhosh-Sellavel solution and @Santhosh-Sellavel solution also need.

cc @Luke9389 @SofiedeVreese

@puneetlath
Copy link
Contributor

@SofiedeVreese @Luke9389 I'm back from vacation now. What @PrashantMangukiya said is correct. I recommended that we split the implementation of the validation and the date picker changes here which is why there are separate issues and they are submitting separate PRs.

@Luke9389
Copy link
Contributor

Luke9389 commented Nov 1, 2021

Ah OK so let me see if I have this straight. We are adding the following line in @PrashantMangukiya's PR because they don't have to use the date picker:

return testDate.isValid() && testDate.isBetween(pastDate, currentDate);

In @Santhosh-Sellavel's PR we'll be making changes to the DatePicker component itself to make future dates unselectable.

Is that correct @puneetlath?

@puneetlath
Copy link
Contributor

That's correct!

@Luke9389
Copy link
Contributor

Luke9389 commented Nov 1, 2021

OK cool!
Sorry to have thrown a wrench in the works here, heh.
@Santhosh-Sellavel please feel free to resume your PR.

@SofiedeVreese
Copy link
Contributor Author

No updates on this: open PR.

@SofiedeVreese
Copy link
Contributor Author

Not overdue, PR is open.

@MelvinBot MelvinBot removed the Overdue label Nov 17, 2021
@puneetlath
Copy link
Contributor

@SofiedeVreese the PR has been merged. Let's make sure to add the company offsite bonus for @Santhosh-Sellavel when you pay this one out. Thanks!

@SofiedeVreese
Copy link
Contributor Author

This job was already paid out as we closed it due to a suspected double-up. But I'll pay out the bonus amount 7 days from successful deploy!

@SofiedeVreese
Copy link
Contributor Author

Deployed to Prod- as the original amount was already paid out, only the Offsite hold bonus is remaining which I'll pay out 7 days from today.

@MelvinBot MelvinBot removed the Overdue label Nov 29, 2021
@SofiedeVreese SofiedeVreese changed the title VBA- disabling future dates in the date picker [HOLD for payment of bonus-only 2021-12-06] VBA- disabling future dates in the date picker Nov 29, 2021
@SofiedeVreese SofiedeVreese added the Awaiting Payment Auto-added when associated PR is deployed to production label Nov 29, 2021
@SofiedeVreese
Copy link
Contributor Author

Hey @Santhosh-Sellavel I've just paid out the remaining bonus amount of $250 for the company offsite hold.

I've paid out the bonus via the original contract.

Closing GH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants