-
Notifications
You must be signed in to change notification settings - Fork 164
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
refactor: Make range calendar selection controllable #526
Conversation
@@ -23,7 +25,7 @@ export default function RangeCalendarScenario() { | |||
|
|||
<RangeCalendar | |||
value={value} | |||
onChange={setValue} | |||
setValue={setValue} |
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.
onChange has become setValue to support full React's setState signature, including update in callback
@@ -97,7 +80,7 @@ export const Grids = ({ | |||
const newMonth = !isSingleGrid && direction === -1 ? addMonths(baseDate, -1) : baseDate; | |||
const nearestBaseDate = getBaseDate(newMonth, isDateEnabled); | |||
|
|||
const newFocusedDate = selectFocusedDate(focusedDate, nearestBaseDate, isDateEnabled); | |||
const newFocusedDate = findDateToFocus(focusedDate, nearestBaseDate, isDateEnabled); |
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.
If function does not make any side effects (actually selecting something), then it is find..., not select...
// This effect "synchronizes" the local state update back up to the parent component. | ||
useEffect(() => { |
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.
React docs explicitly call this an anti-pattern: https://beta.reactjs.org/learn/you-might-not-need-an-effect#notifying-parent-components-about-state-changes
Avoid code patterns like this
import { formatDate } from './format-date'; | ||
import { formatTime } from './format-time'; | ||
|
||
export function formatDateTime(date: Date) { |
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.
Very surprised we did not have this function yet. Did I miss something?
@@ -4,3 +4,8 @@ | |||
export function joinDateTime(dateString: string, timeString: string) { | |||
return `${dateString}T${timeString}`; | |||
} | |||
|
|||
export function splitDateTime(dateStr: string) { |
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.
Taking suggestions how to rename the file, since now it includes both ways, split and join
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.
Since we seem to be splitting every function into its own file, this could also go into slit-date-time.ts
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.
I would prefer having symmetric functions in a single file (because they are always changed together)
8841b51
to
57ae6d9
Compare
Codecov ReportBase: 92.61% // Head: 92.59% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #526 +/- ##
==========================================
- Coverage 92.61% 92.59% -0.02%
==========================================
Files 567 568 +1
Lines 16131 16127 -4
Branches 4417 4411 -6
==========================================
- Hits 14939 14933 -6
- Misses 1112 1114 +2
Partials 80 80
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Description
Move state into a single place in
DateRangePickerDropdown
component. This will allow to expose programmatic API to control this value outside the componentRelated links, issue #, if available: n/a
How has this been tested?
PR build, dev-pipeline (link in DM)
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.