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

Pass invalid values to the onDateChange prop #1937

Open
marcgarygray opened this issue Mar 2, 2020 · 7 comments · May be fixed by #1941
Open

Pass invalid values to the onDateChange prop #1937

marcgarygray opened this issue Mar 2, 2020 · 7 comments · May be fixed by #1941

Comments

@marcgarygray
Copy link

I believe it would be helpful to a lot of developers to be able to know if an invalid date has been typed by the user into the <input type="text"> node. In my opinion, the most logical place to provide that information would be in the onDateChange handler. I could see an argument for a simple boolean that's false if the input is not a valid moment.Moment or passing the invalid input as a string.
I know that an invalid input just passes null as the date to the handler, but that being the case, there's no way to differentiate between the user simply clearing the data (a valid interaction) versus entering text that can't be parsed into a valid date (an invalid interaction).

If you allow pull requests from the masses, I would be more than happy to make the change and submit a PR.

@marcgarygray
Copy link
Author

Note

This is for the <SingleDatePicker> specifically. It's the only component from this library that I've utilized thus far. I'm not sure if this is the same or different across all of the components exported by the library.

@ljharb
Copy link
Member

ljharb commented Mar 3, 2020

If there's a nonbreaking way to also pass the original input string, I'd be open to that.

I think this might be a duplicate of #1023?

@marcgarygray
Copy link
Author

@ljharb
Are you open to outside contributions? I'm happy to make the change.
If so, any guidelines (PR must come with a test, etc., etc.) are appreciated. :)

@ljharb
Copy link
Member

ljharb commented Mar 3, 2020

Yes, absolutely! Tests are a requirement; a storybook story is nice but not required.

It'd be ideal to make this change on every component in the library, so that everything can follow similar patterns.

@marcgarygray
Copy link
Author

@ljharb working on this tonight.
Looks like your workflow is to create a branch named something like username/issue#/description and make a PR to merge that branch to master, yeah?

@ljharb
Copy link
Member

ljharb commented Mar 5, 2020

The workflow in general terms for making a PR to any open source project is to fork the repo, make a branch on the fork with your changes, and then make a PR to merge that to the source's default branch (usually master).

@marcgarygray
Copy link
Author

@ljharb thanks. Submitted. 👍

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 a pull request may close this issue.

2 participants