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

[6.0] DatePicker: cherry-pick "utilize custom onChange handler when provided #10424" #10478

Merged

Conversation

natalieethell
Copy link
Contributor

@natalieethell natalieethell commented Sep 17, 2019

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Description of changes

Cherry-pick changes from master (#10424) that utilize the custom onChange handler when provided.

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

@natalieethell natalieethell requested a review from a team as a code owner September 17, 2019 17:27
@natalieethell natalieethell changed the title [6.0] DatePicker: cherry-pick utilize custom onChange handler when provided [6.0] DatePicker: cherry-pick "utilize custom onChange handler when provided " Sep 17, 2019
@natalieethell natalieethell changed the title [6.0] DatePicker: cherry-pick "utilize custom onChange handler when provided " [6.0] DatePicker: cherry-pick "utilize custom onChange handler when provided #10424" Sep 17, 2019
@size-auditor
Copy link

size-auditor bot commented Sep 17, 2019

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react DatePicker 200.087 kB 200.143 kB ExceedsBaseline     56 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: b41213bb5655812b2d5469a24139f74e7e015e65 (build)

@dzearing
Copy link
Member

dzearing commented Sep 18, 2019

@lorejoh12 @natalieethell it seems odd that we are imperatively calling onChange on the TextField from the DatePicker. It seems like there should be a more declarative way to do this. I won't block the cherrypick, as I will defer to the Calendar folks, but this seems a little off to me.

For example, onChange on the TextField probably should only fire when a user input happens and the TextField wants the user to change the value. If the DatePicker passes a new value prop to the TextField, I would not expect TextField.onChange to fire, as it isn't user input. I could however expect DatePicker.onChange to fire in a controlled DatePicker scenario.

The caller then listens to DatePicker onChange instead of DatePicker's TextField.onChange.

That way the caller doesn't need to reach into the inner component within the DatePicker and instead treats the DatePicker like a TextField.

Let me know what you think. I might be misunderstanding the core scenario.

@lorejoh12
Copy link
Contributor

@lorejoh12 @natalieethell it seems odd that we are imperatively calling onChange on the TextField from the DatePicker. It seems like there should be a more declarative way to do this. I won't block the cherrypick, as I will defer to the Calendar folks, but this seems a little off to me.
For example, onChange on the TextField probably should only fire when a user input happens and the TextField wants the user to change the value. If the DatePicker passes a new value prop to the TextField, I would not expect TextField.onChange to fire, as it isn't user input. I could however expect DatePicker.onChange to fire in a controlled DatePicker scenario.
The caller then listens to DatePicker onChange instead of DatePicker's TextField.onChange.
That way the caller doesn't need to reach into the inner component within the DatePicker and instead treats the DatePicker like a TextField.
Let me know what you think. I might be misunderstanding the core scenario.

I don't have a strong opinion on whether the consumer should be binding to the textField.onChange vs the DatePicker's own onChange, but your suggestion seems reasonable from an API perspective. Do textboxes onChange not normally react to external inputs changing them? I think I've seen some in OWA that do, specifically autofill from suggestion drop down, but it could have been the onChange for the whole control and not the textbox specifically.

@msft-github-bot
Copy link
Contributor

Hello @natalieethell!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msft-github-bot) and give me an instruction to get started! Learn more here.

@msft-github-bot msft-github-bot merged commit ec57d0a into microsoft:6.0 Sep 19, 2019
@msft-github-bot
Copy link
Contributor

🎉office-ui-fabric-react@v6.204.1 has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot
Copy link
Contributor

🎉@uifabric/date-time@v6.3.8 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants