[LG-5504] feat(input-box): add InputSegment
#3293
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…associated types and tests
…egment component references
|
3 tasks
shaneeza
commented
Nov 7, 2025
Collaborator
Author
There was a problem hiding this comment.
I think i should break these out into smaller files
Collaborator
There was a problem hiding this comment.
👍 Not necessary, but wouldn't hurt
shaneeza
requested review from
a team,
TheSonOfThomp and
stephl3
and removed request for
a team
3 tasks
Contributor
|
Size Change: +1.5 kB (+0.08%) Total Size: 1.79 MB
ℹ️ View Unchanged
|
5 tasks
shaneeza
commented
Nov 7, 2025
shaneeza
commented
Nov 7, 2025
shaneeza
commented
Nov 7, 2025
| step?: number; | ||
|
|
||
| /** | ||
| * Whether the segment should wrap at min/max boundaries |
Collaborator
Author
There was a problem hiding this comment.
need to update this: should only be at the max boundary
…se object parameter format for improved clarity and consistency
…ity by destructuring context values
…utBoxTypes and remove deprecated InputSegment types
stephl3
reviewed
Nov 10, 2025
Comment on lines
4
to
5
| import { InputBoxProvider } from '../InputBoxContext/InputBoxContext'; | ||
| import { InputBoxProviderProps } from '../InputBoxContext/InputBoxContext.types'; |
Collaborator
There was a problem hiding this comment.
Suggested change
| import { InputBoxProvider } from '../InputBoxContext/InputBoxContext'; | |
| import { InputBoxProviderProps } from '../InputBoxContext/InputBoxContext.types'; | |
| import { InputBoxProvider, type InputBoxProviderProps } from '../InputBoxContext/InputBoxContext'; |
Comment on lines
6
to
7
| import { InputSegment } from '../InputSegment/InputSegment'; | ||
| import { InputSegmentProps } from '../InputSegment/InputSegment.types'; |
Collaborator
There was a problem hiding this comment.
Suggested change
| import { InputSegment } from '../InputSegment/InputSegment'; | |
| import { InputSegmentProps } from '../InputSegment/InputSegment.types'; | |
| import { InputSegment, type InputSegmentProps } from '../InputSegment/InputSegment'; |
| /** | ||
| * Which segment this input represents | ||
| * | ||
| * @example |
Collaborator
There was a problem hiding this comment.
should this include time segment examples?
| export interface InputSegmentProps<Segment extends string> | ||
| extends Omit< | ||
| React.ComponentPropsWithRef<'input'>, | ||
| 'size' | 'step' | 'value' |
Collaborator
There was a problem hiding this comment.
why do we omit 'size' and 'value'?
Collaborator
Author
There was a problem hiding this comment.
These values are read from the context, not from the prop
| * | ||
| * @default false | ||
| */ | ||
| shouldSkipValidation?: boolean; |
Collaborator
There was a problem hiding this comment.
Is it worth refactoring out this double negative at this point? i.e. shouldValidate w/ default of true vs shouldSkipValidation w/ default of false? shouldValidate nicely maintains parallel structure with the shouldWrap boolean prop
Collaborator
Author
There was a problem hiding this comment.
I'd like to have a parallel structure. I'll update!
| min, | ||
| max, | ||
| step, | ||
| shouldWrap: shouldWrap, |
Collaborator
There was a problem hiding this comment.
Suggested change
| shouldWrap: shouldWrap, | |
| shouldWrap, |
Comment on lines
196
to
197
| onBlur?.(e); | ||
| onBlurProp?.(e); |
Collaborator
There was a problem hiding this comment.
This looks a little strange to me. I like that onBlurProp has the explicit *Prop suffix. Anything we can add to onBlur to make it clear that is from whatever is specified by the onBlur in InputBox?
Collaborator
Author
There was a problem hiding this comment.
Maybe onBlurContextProp?
| } | ||
| `; | ||
|
|
||
| export const segmentThemeStyles: Record<Theme, string> = { |
Collaborator
There was a problem hiding this comment.
opportunity to refactor to tokens and rm palette dep
|
|
||
| import { InputSegment, InputSegmentChangeEventHandler } from '.'; | ||
|
|
||
| describe('packages/input-segment', () => { |
Collaborator
There was a problem hiding this comment.
worth adding a jest-axe test block?
…reen-ui into LG-5504/input-box-segment
…ared types for better organization
…maxSegmentValue for consistency
… violations when tooltip is closed
| } as InputBoxProviderProps<SegmentObjMock>; | ||
|
|
||
| const utils = render( | ||
| <InputBoxProvider {...mergedProviderProps}> |
Collaborator
There was a problem hiding this comment.
Do the InputSegment tests need to depend on the InputBoxProvider?
I'd rather test the segment in isolation if possible
Comment on lines
133
to
143
| const { input } = renderSegment({ | ||
| providerProps: { | ||
| segments: { day: '26', month: '', year: '' }, | ||
| onChange: onChangeHandler, | ||
| }, | ||
| }); | ||
|
|
||
| userEvent.type(input, '4'); | ||
| expect(onChangeHandler).toHaveBeenCalledWith( | ||
| expect.objectContaining({ value: '4' }), | ||
| ); |
Collaborator
There was a problem hiding this comment.
it's not really clear here in the tests why typing 4 into a segment that has 26 would just render 4 instead of 264. There's logic baked in somewhere that isn't entirely clear based on this test (I'm assuming there's a default min/max value?)
Comment on lines
+150
to
+151
| charsPerSegment: charsPerSegmentMock['day'], | ||
| allowZero: defaultMinMock['day'] === 0, |
Collaborator
There was a problem hiding this comment.
nit: Imo we can remove the charsPerSegment mock, and just hard code values for the tests
Suggested change
| charsPerSegment: charsPerSegmentMock['day'], | |
| allowZero: defaultMinMock['day'] === 0, | |
| charsPerSegment: 2, | |
| allowZero: true, |
Comment on lines
44
to
54
| const { | ||
| onChange: onChangeContextProp, | ||
| onBlur: onBlurContextProp, | ||
| charsPerSegment: charsPerSegmentContext, | ||
| segmentEnum, | ||
| segmentRefs, | ||
| segments, | ||
| labelledBy, | ||
| size, | ||
| disabled, | ||
| } = useInputBoxContext<Segment>(); |
Collaborator
There was a problem hiding this comment.
I feel like it might be simpler if these were just props to the segment
Collaborator
There was a problem hiding this comment.
👍 Not necessary, but wouldn't hurt
Comment on lines
97
to
115
| decorator: (StoryFn, context) => ( | ||
| <LeafyGreenProvider darkMode={context?.args.darkMode}> | ||
| <InputBoxProvider | ||
| charsPerSegment={charsPerSegmentMock} | ||
| segmentEnum={SegmentObjMock} | ||
| onChange={() => {}} | ||
| onBlur={() => {}} | ||
| segmentRefs={segmentRefsMock} | ||
| segments={context?.args.segments} | ||
| size={context?.args.size} | ||
| disabled={false} | ||
| > | ||
| <StoryFn | ||
| placeholder={ | ||
| defaultPlaceholderMock[context?.args.segment as SegmentObjMock] | ||
| } | ||
| /> | ||
| </InputBoxProvider> | ||
| </LeafyGreenProvider> |
Collaborator
There was a problem hiding this comment.
Yeah, having half the props come from the InputBoxProvider and the other as real props is probably overly complex for this component
Collaborator
Author
There was a problem hiding this comment.
Chatted in person, and I'm going to remove the context and pass the props directly to the Segment component in InputBox
…nce type definitions for better clarity
…d onBlur event handlers with detailed documentation
…nd include additional props for improved functionality
…amline input box functionality
shaneeza
changed the base branch from
LG-5504/input-box-context
to
shaneeza/time-picker-integration
…neric and updating related components for improved clarity
…and remove unused segmentRefs and segments properties for improved clarity
shaneeza
commented
Nov 12, 2025
Collaborator
Author
There was a problem hiding this comment.
This was inherited from the first context PR that I closed. The tests were updated to wrap the props in an object and a new test was added to test the new allowZero prop
…/leafygreen-ui into LG-5504/input-box-segment
stephl3
approved these changes
Nov 13, 2025
| const isIncomingValueNumber = !isNaN(Number(incomingValue)); | ||
| // macOS adds a period when pressing SPACE twice inside a text input. | ||
| const doesIncomingValueContainPeriod = /\./.test(incomingValue); | ||
| const shouldSkipValidation = !shouldValidate; |
Collaborator
There was a problem hiding this comment.
nit: I think !shouldValidate is readable as-is. Feels more confusing to re-assign to this shouldSkipValidation variable
Comment on lines
57
to
62
| * 'day' | ||
| * 'month' | ||
| * 'year' | ||
| * 'hours' | ||
| * 'minutes' | ||
| * 'seconds' |
Collaborator
There was a problem hiding this comment.
date uses singular while time uses plural. can we make these consistent?
Comment on lines
16
to
26
| /** | ||
| * Minimum value for the segment | ||
| * | ||
| * @example | ||
| * 1 | ||
| * 1 | ||
| * 1970 | ||
| * 0 | ||
| * 0 | ||
| * 0 | ||
| */ |
Collaborator
There was a problem hiding this comment.
nit: doesn't seem all that helpful to list numbers here in this format. maybe makes more sense to restrict to unique values?
Collaborator
Author
There was a problem hiding this comment.
will remove these becuase I don't think they are that helpful
Comment on lines
29
to
39
| /** | ||
| * Maximum value for the segment | ||
| * | ||
| * @example | ||
| * 31 | ||
| * 12 | ||
| * 2038 | ||
| * 23 | ||
| * 59 | ||
| * 59 | ||
| */ |
Collaborator
There was a problem hiding this comment.
nit: similar to above, these feel like arbitrary values when presented without the broader context
Comment on lines
73
to
80
| /** | ||
| * The number of characters per segment | ||
| * | ||
| * @example | ||
| * 2 | ||
| * 2 | ||
| * 4 | ||
| */ |
Collaborator
There was a problem hiding this comment.
nit: similar to above, these feel like arbitrary values when presented without the broader context
| "access": "public" | ||
| }, | ||
| "dependencies": { | ||
| "@leafygreen-ui/a11y": "workspace:^", |
Collaborator
There was a problem hiding this comment.
might be fine to drop the palette dependency altogether at this point
stephl3
reviewed
Nov 13, 2025
| }; | ||
| export default meta; | ||
|
|
||
| export const LiveExample: StoryFn<typeof InputSegment> = ({ |
Collaborator
There was a problem hiding this comment.
probably can disable this snapshot since you have a generated story
…nt types for consistency
Collaborator
TheSonOfThomp
left a comment
TheSonOfThomp
left a comment
There was a problem hiding this comment.
Some nits, but nothing blocking
| onChange: () => {}, | ||
| onBlur: () => {}, | ||
| value: '', | ||
| charsPerSegment: charsPerSegmentMock['day'], |
Collaborator
There was a problem hiding this comment.
nit: we probably don't need the charsPerSegmentMock and defaultMinMock, and could just pass the values directly
| /** | ||
| * The number of characters per segment | ||
| */ | ||
| charsPerSegment: number; |
Collaborator
There was a problem hiding this comment.
charsPerSegment is a bit of a redundant name in the abstract context. Just a simple charsCount or something would be sufficient here
Comment on lines
19
to
24
| minSegmentValue: number; | ||
|
|
||
| /** | ||
| * Maximum value for the segment | ||
| * | ||
| * @example | ||
| * 31 | ||
| * 12 | ||
| * 2038 | ||
| * 23 | ||
| * 59 | ||
| * 59 | ||
| */ | ||
| maxSegmentValue: number; |
Collaborator
There was a problem hiding this comment.
This is fine for now, but we should note that it may be desirable to let this support alphanumeric chars in the future (and so min/max values don't make sense)
Comment on lines
57
to
62
| * 'day' | ||
| * 'month' | ||
| * 'year' | ||
| * 'hour' | ||
| * 'minute' | ||
| * 'second' |
Collaborator
There was a problem hiding this comment.
per stephen's other comment, we can rm this
Comment on lines
78
to
80
| * '1' | ||
| * '2' | ||
| * '2025' |
| * | ||
| * These types are shared between the InputBox and the segmentComponent. | ||
| */ | ||
| export interface SharedInputBoxTypes<Segment extends string> { |
Collaborator
There was a problem hiding this comment.
is this still needed here?
Collaborator
Author
There was a problem hiding this comment.
I plan on using these types in InputBox as well
…ency across InputSegment components
Contributor
|
Coverage after merging LG-5504/input-box-segment into shaneeza/time-picker-integration will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
shaneeza
merged commit 4059f91
into
shaneeza/time-picker-integration
12 of 13 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
This PR is the first PR in a series of three PRs that adds the new
InputBoxpackage and integrates it into the existingDatePickerpackage:InputBox#3285InputBoxcomponent #3286This PR adds the generic
InputSegmentcomponent to the new @leafygreen-ui/input-box package, which provides reusable input segment functionality for date and time components. The component extracts common logic and UI elements previously embedded in the date-picker package, making it available for use across DatePicker, TimeInput, and other similar components.🎟 Jira ticket: LG-5504
✅ Checklist
For new components
🧪 How to test changes
InputSegmentcomponent renders correctly in their respective stories