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

[DatePicker] textField prop: onChange is never being called #10406

Closed
watty888 opened this issue Sep 10, 2019 · 13 comments · Fixed by #10424
Closed

[DatePicker] textField prop: onChange is never being called #10406

watty888 opened this issue Sep 10, 2019 · 13 comments · Fixed by #10424
Assignees

Comments

@watty888
Copy link

Environment Information

  • Package version(s): 6.202.0
  • React: 16.9.0
  • Browser: Chrome, Version 76.0.3809.132 (Official Build) (64-bit)
  • OS: macOS Mojave 10.14.6 (18G95)

Bug reproduction in codesandbox:

Steps:

  1. Check the index.tsx file
  2. Clear console
  3. Start typing in the textfield

Actual behavior:

Upon typing something in the text field, entered value is not being logged into console.
onChange of the textField prop is never being called.

Screen Recording 2019-09-10 at 14 44 00

Expected behavior:

Input of the text field must appear in the console as you type. onChange should call console.log.

Priorities and help requested:

Are you willing to submit a PR to fix? (No)

Requested priority: (High)

@watty888 watty888 changed the title DatePicker textField prop: onChange is never being called DatePicker: textField prop: onChange is never being called Sep 10, 2019
@watty888 watty888 changed the title DatePicker: textField prop: onChange is never being called [DatePicker] textField prop: onChange is never being called Sep 10, 2019
@natalieethell
Copy link
Contributor

Hey @watty888 thanks for opening an issue! It looks like this is happening, because the onChange method on the text field is actually always overridden by the onChange method in the DatePicker code itself (note lines 190 and 203 below).

https://github.com/OfficeDev/office-ui-fabric-react/blob/72e835bd59d6c29b1a222fccdd5c57a339c73090/packages/office-ui-fabric-react/src/components/DatePicker/DatePicker.base.tsx#L190-L203

@evlevy it looks like you made the changes to add the textField prop for increased customizability. Was this placement intentional? Also @lorejoh12 FYI.

Thank you also for providing a code sandbox for us, that makes it a lot easier to pinpoint and debug the issue much faster!

@evlevy
Copy link
Contributor

evlevy commented Sep 11, 2019

@natalieethell at the time textFieldProps was introduced we didn't consider folks wanting to override onChange - it was more for customizing the UI of the textField and not the behaviour. If this is now required, we would need to consider which handler to call, or possibly integrate the supplied handler into the existing one.

@evlevy evlevy closed this as completed Sep 11, 2019
@evlevy evlevy reopened this Sep 11, 2019
@evlevy
Copy link
Contributor

evlevy commented Sep 11, 2019

(Sorry I accidentally closed this issue just now. Reopened.)

@natalieethell
Copy link
Contributor

@evlevy Got it, maybe it would be good to integrate the supplied handler into the existing onChange handler. I'll take a closer look. Thanks!

@natalieethell
Copy link
Contributor

@evlevy Opened a PR with those changes, feel free to take a look if you have a moment.

@watty888
Copy link
Author

@natalieethell thanks for your rush reply! Can you also please specify for which version your fix will be applied? The project we are currently working at uses 6.202.0.

@natalieethell
Copy link
Contributor

@watty888 This change will go into the master (7.x.x) branch, but we can also backport the changes to 6.x.x.

@watty888
Copy link
Author

but we can also backport the changes to 6.x.x.

That would be great, as in the projects current state, we would prefer not to update for at least some time.

@msft-github-bot
Copy link
Contributor

🎉This issue was addressed in #10424, which has now been successfully released as @uifabric/date-time@v7.4.5.:tada:

Handy links:

@msft-github-bot
Copy link
Contributor

🎉This issue was addressed in #10424, which has now been successfully released as office-ui-fabric-react@v7.34.0.:tada:

Handy links:

@watty888
Copy link
Author

Hi Natalie! Great that it's fixed! However, are there any updates regarding 6.x.x version fix?

@natalieethell
Copy link
Contributor

@watty888 Thanks for the reminder! I'll cherry pick into 6.0 right now.

@natalieethell
Copy link
Contributor

You can follow the cherry-pick into 6.0 here: #10478.

@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants