-
Notifications
You must be signed in to change notification settings - Fork 72
[LG-5504] refactor(date-picker): migrate to use InputBox component
#3286
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
[LG-5504] refactor(date-picker): migrate to use InputBox component
#3286
Conversation
… improved type handling and event management
…tSegment with additional props
…with improved type handling and segment management
…ved segment management and type handling
…es for better segment management
…pe handling and segment management
… new props for step handling and rollover management
…icker components for improved segment management and type handling
…nce segment validation logic for improved date handling
…ean up InputBox component
…alidation and formatting functions
…tSegment and InputBox tests for better coverage
…ent components by introducing utility functions
…gment components for improved clarity and documentation
…ent types with clearer examples
…Rules in InputBox types for better clarity
…ting getValueFormatter to accept segment-specific character counts
… for year segment and enhance validation logic
…ype safety and clarity
…ng and props validation
…r consistency and clarity across components
… tests for InputBox and InputSegment components
…n testutils for enhanced testing capabilities
…nputBox stories with date and time segment examples
…stutils for cleaner code
…ging in InputBox component
…ygreen-ui into LG-5504/input-box-in-date-picker-3
…elated references to charsCount for consistency
…ate DateInputBoxProviderProps to extend PropsWithChildren for better type handling
…-digit year value in custom validation for date segments
…tead of charsPerSegment across components and tests for consistency
| <InputBox | ||
| ref={fwdRef} | ||
| segmentRefs={segmentRefs} | ||
| segmentEnum={DateSegment} | ||
| formatParts={formatParts} | ||
| segments={segments} | ||
| setSegment={setSegment} | ||
| disabled={disabled} | ||
| segmentRules={dateSegmentRules} | ||
| onSegmentChange={onSegmentChange} | ||
| labelledBy={labelledBy} | ||
| segmentComponent={DateInputSegment} | ||
| {...rest} | ||
| /> |
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.
re: previous comment on pass-through props, segmentRefs, onSegmentChange, and labelledBy look like they can also be passed through. I think this has the benefit of making it clearer which props are provided from DatePicker context or derived by other means
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.
Also, it looks like className is no longer being considered. Is that correct?
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.
Whoops, I missed these. Thanks.
…for cleaner implementation
…ableSnapshot in DatePicker stories and pass size prop to InputBox
| value, | ||
| }: PropsWithChildren<DateInputBoxProviderProps>) => { | ||
| return ( | ||
| <DateInputBoxContext.Provider value={{ value }}> |
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: it's a bit confusing to have the DateInputBoxProvider prop named value that's different from the DateInputBoxContext.Provider value prop
| const { | ||
| autoComplete: autoCompleteProp, | ||
| min: minContextProp, | ||
| max: maxContextProp, | ||
| } = useSharedDatePickerContext(); | ||
|
|
||
| const { value: dateValue } = useDateInputBoxContext(); |
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.
My intuition says the dateValue should be on the useSharedDatePickerContext. Is it just not needed until the DateInputBoxContext?
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 think the date value isn't stored in the SharedDatePickerProvider because this provider was built with the date range picker in mind. I believe because of the range picker, there can be multiple DateInputBox components within a SharedDatePickerProvider, and each component needs its own separate value, which DateInputSegment relies on. So storing a single value in the shared provider might not be correct.
Does that sound familiar? If any of that is incorrect, then I can add the value to the shared provider.
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.
Ah that makes sense. We might need a "mid-level" provider, but we can cross that bridge later
| ): boolean => { | ||
| return Object.entries(segments).every(([segment, value]) => | ||
| isExplicitSegmentValue(segment as DateSegment, value), | ||
| isExplicitSegmentValue({ segment: segment as DateSegment, value }), |
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.
Why is segment not automatically typed as DateSegment here?
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.
It looks like Object.entries narrows the key type to string because when Object.entries runs, the key could be any string, so we need to assert the correct type.
…/leafygreen-ui into LG-5504/input-box-in-date-picker-3
…/leafygreen-ui into LG-5504/input-box-in-date-picker-3
…d and mouse interaction specs
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
| afterAll(() => { | ||
| jest.useRealTimers(); | ||
| }); |
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.
Should we be doing useRealTimers in afterEach or afterAll? I'm not sure, but I noticed you have it in both in different files
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.
We added the fake timers in beforeAll, so It's mirroring that
…e` in DateInputBox components
|
Coverage after merging LG-5504/input-box-in-date-picker-3 into shaneeza/time-picker-integration will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
✍️ Proposed changes
This PR is the third PR in a series of three PRs that add the new
InputBoxpackage and integrate it into the existingDatePickerpackage:InputSegment#3293InputBox#3285This PR refactors the date-picker package to leverage the new
@leafygreen-ui/input-boxcomponent instead of maintaining custom input segment implementations. This migration significantly simplifies the codebase by removing redundant logic and utilities, while maintaining the same functionality through the shared input-box component.Key changes include:
DateInputSegmentimplementation withInputSegmentfrom@leafygreen-ui/input-box🎟️ Jira ticket: LG-5504
✅ Checklist
pnpm changesetand documented my changes🧪 How to test changes