-
Notifications
You must be signed in to change notification settings - Fork 2k
Calendar components: simplify types, forward all onChange
arguments
#103675
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
onChange
arguments
14bf8d6
to
57c9895
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the cleanup 👍
* console.log("Selected:", selected); | ||
* console.log("Triggered by:", triggerDate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: do we care about WP-style spacing in these kinds of examples?
57c9895
to
1c3decc
Compare
E2E can't be caused by the changes in this PR since they are not exported from the package (and used anywhere) |
Closes DS-232
Proposed Changes
required
prop.onChange
arguments in theuseControlledValue
hookWhy are these changes being made?
The current type unions complicate the code without necessarily bringing better DX. Removing them allows us to simplify the code, and generalize the
useControlledValue
hook.The arguments forwarding for the
onChange
callback inuseControlledValue
also fixes an inconsistency, where theonSelect
callback prop would return extra arguments only in uncontrolled mode.Testing Instructions
DateCalendar
andDateRangeCalendar
works as expected in StorybookPre-merge Checklist