Skip to content

Conversation

@shaneeza
Copy link
Collaborator

@shaneeza shaneeza commented Nov 19, 2025

✍️ Proposed changes

This PR adds an exportable util that provides segment focus functionality, which was initially missed when extracting logic from DatePicker into InputBox. However, this functionality will not be used directly in InputBox.

This util should be used in an onClick callback in the element wrapping the InputBox component. This is necessary because the wrapping element will contain the height/padding, and we need that height to be included in the clickable area for proper focus behavior.

For example, in DatePicker, this util is used within DateInputInput's onClick handler, since DateInputInput consumes DateInputBox, which in turn consumes InputBox.

🎟 Jira ticket: Name of ticket

✅ Checklist

For bug fixes, new features & breaking changes

  • I have added stories/tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have run pnpm changeset and documented my changes

… improved type handling and event management
…with improved type 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
…tSegment and InputBox tests for better coverage
…ent components by introducing utility functions
…gment components for improved clarity and documentation
…ting getValueFormatter to accept segment-specific character counts
… for year segment and enhance validation logic
… tests for InputBox and InputSegment components
@shaneeza shaneeza changed the base branch from shaneeza/time-picker-integration to LG-5504/input-box-in-date-picker-3 November 20, 2025 17:11
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved from DatePicker with git mv but I think renaming the file ended up making this a new file

@shaneeza shaneeza marked this pull request as ready for review November 20, 2025 23:32
@shaneeza shaneeza requested a review from a team as a code owner November 20, 2025 23:32
@shaneeza shaneeza requested a review from Copilot November 20, 2025 23:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR extracts and generalizes segment focus functionality from DatePicker into reusable utilities in the InputBox package. The changes enable other components to implement segment-based input focus behavior without duplicating the logic.

Key Changes:

  • Moved getFirstEmptySegment and getSegmentToFocus utilities from DatePicker to InputBox with generic typing
  • Added focusAndSelectSegment convenience wrapper that combines focus logic with DOM manipulation
  • Updated DatePicker to consume the new utilities from InputBox

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/input-box/src/utils/index.ts Exports new utility functions for segment focus management
packages/input-box/src/utils/getSegmentToFocus/getSegmentToFocus.ts Moved and generalized with optional parameters and improved typing
packages/input-box/src/utils/getSegmentToFocus/getSegmentToFocus.spec.tsx Comprehensive test coverage for segment focus logic
packages/input-box/src/utils/getFirstEmptySegment/getFirstEmptySegment.ts Extracted utility to find first empty segment with generic typing
packages/input-box/src/utils/getFirstEmptySegment/getFirstEmptySegment.spec.ts Test coverage for empty segment detection
packages/input-box/src/utils/focusAndSelectSegment/focusAndSelectSegment.ts New convenience wrapper combining focus and select operations
packages/input-box/src/utils/focusAndSelectSegment/focusAndSelectSegment.spec.tsx Tests for the focus and select wrapper
packages/input-box/src/index.ts Updated exports to include new utilities
packages/date-picker/src/shared/utils/index.ts Removed export of old getFirstEmptySegment
packages/date-picker/src/shared/utils/getFirstEmptySegment/index.ts Deleted file after extraction to InputBox
packages/date-picker/src/DatePicker/utils/getSegmentToFocus/getSegmentToFocus.spec.ts Removed tests now covered in InputBox
packages/date-picker/src/DatePicker/DatePickerInput/DatePickerInput.tsx Updated to use focusAndSelectSegment from InputBox


import { getSegmentToFocus, SegmentRefsType } from './getSegmentToFocus';

describe('packages/date-picker/utils/getSegmentToFocus', () => {
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The describe block references 'date-picker' but this test is now in the 'input-box' package. Update to 'packages/input-box/utils/getSegmentToFocus'.

Suggested change
describe('packages/date-picker/utils/getSegmentToFocus', () => {
describe('packages/input-box/utils/getSegmentToFocus', () => {

Copilot uses AI. Check for mistakes.
Base automatically changed from LG-5504/input-box-in-date-picker-3 to shaneeza/time-picker-integration November 21, 2025 01:26
…/leafygreen-ui into LG-5504/input-box-segment-focus
Copy link
Collaborator

@stephl3 stephl3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add some interaction tests to capture the expected visual focus styles when a particular segment is clicked/focused?

{ type: 'day', value: '' },
];

test('focuses and selects the target segment', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any other cases we want to test beyond the happy path?

@shaneeza
Copy link
Collaborator Author

can we add some interaction tests to capture the expected visual focus styles when a particular segment is clicked/focused?

@stephl3 Individual segments don't have focus styles. It's up to the consuming component to add a focus outline to the input container with focus-within. I believe that the tests in focusAndSelectSegment are sufficient because they verify that focus() is called. What do you think?

@shaneeza shaneeza requested a review from stephl3 November 21, 2025 20:37
@stephl3
Copy link
Collaborator

stephl3 commented Nov 21, 2025

can we add some interaction tests to capture the expected visual focus styles when a particular segment is clicked/focused?

@stephl3 Individual segments don't have focus styles. It's up to the consuming component to add a focus outline to the input container with focus-within. I believe that the tests in focusAndSelectSegment are sufficient because they verify that focus() is called. What do you think?

I see some focus styles being set in InputSegment.styles.ts. Are those unintentional?
Screenshot 2025-11-21 at 3 18 18 PM

@shaneeza
Copy link
Collaborator Author

shaneeza commented Nov 22, 2025

can we add some interaction tests to capture the expected visual focus styles when a particular segment is clicked/focused?

@stephl3 Individual segments don't have focus styles. It's up to the consuming component to add a focus outline to the input container with focus-within. I believe that the tests in focusAndSelectSegment are sufficient because they verify that focus() is called. What do you think?

I see some focus styles being set in InputSegment.styles.ts. Are those unintentional?

@stephl3 Ahh you're correct. I had it in my mind that that was a native browser background color. In that case, I've updated the generated story to include focus styles.

@github-actions
Copy link
Contributor

Coverage after merging LG-5504/input-box-segment-focus into shaneeza/time-picker-integration will be

79.05%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
charts/chart-card/src/ChartCard
   ChartCardContext.tsx60%100%50%66.67%25
charts/colors/src
   colors.ts0%100%100%0%25, 44, 5
charts/core/src
   constants.ts50%100%0%66.67%13
charts/core/src/Axis
   Axis.ts0%0%0%0%15, 19, 21, 23, 23, 45, 45, 61
   XAxis.tsx0%0%0%0%15, 19, 31, 34, 34, 58–60, 62–63, 63, 63, 65, 67, 71, 78
   YAxis.tsx0%0%0%0%15, 19, 30, 33, 33, 57–59, 61–62, 62, 62, 64, 66, 70, 77
charts/core/src/Chart
   Chart.tsx81.25%66.67%100%100%58, 64, 95
charts/core/src/Chart/config
   getDefaultChartOptions.ts100%100%100%100%
charts/core/src/Chart/hooks
   useChart.ts77.01%82.14%76.92%73.91%118, 118, 118–119, 123, 123, 123–125, 128, 132, 136–137, 139–140, 92, 98
   useTooltipVisibility.ts57.24%40%56%65.52%109–110, 120, 133, 133–135, 145–146, 149–151, 166, 166, 166–167, 170, 170, 170–171, 171, 174–176, 190, 190, 190–191, 194, 194, 194–195, 195, 198–199, 224, 224, 228–229, 231–234, 47, 56, 56, 56, 56, 56–57, 60–61, 79–80
charts/core/src/ChartContext
   ChartContext.tsx83.33%50%100%87.50%21–22
charts/core/src/ChartHeader
   ChartHeader.tsx100%100%100%100%
charts/core/src/ChartTooltip
   ChartTooltip.tsx0%0%0%0%100–101, 105–106, 106, 106, 108, 111, 111, 115, 115, 115, 143, 143, 147–148, 163, 22, 35–36, 38, 40, 64, 70–71, 71, 71, 77, 82–83, 87, 87, 87–88, 92, 97–98
   utils.ts100%100%100%100%
charts/core/src/ChartTooltip/CustomTooltip
   CustomTooltip.testUtils.ts0%100%100%0%3
   CustomTooltip.tsx78.26%66.67%100%88.89%46–47, 54, 60, 69
charts/core/src/ChartTooltip/CustomTooltip/SeriesList
   SeriesList.tsx90.91%83.33%100%91.67%15, 19
charts/core/src/ChartTooltip/CustomTooltip/SeriesListItem
   SeriesListItem.tsx90%87.50%100%90.91%27–28
charts/core/src/ChartTooltip/CustomTooltip/SeriesListItemColorDot
   SeriesListItemColorDot.tsx100%100%100%100%
charts/core/src/Echart
   initializeEcharts.ts85.19%75%100%85.71%30–31, 73–74
   updateUtils.tsx0%0%0%0%10, 12, 12, 12–13, 17, 24, 26, 26, 26–28, 32, 46, 48–49, 49, 49–50, 50, 54, 57, 61, 68, 7, 9, 9, 9
   useEchart.ts53.51%41.11%67.86%58.17%109–110, 117, 124, 124, 124, 126, 126, 126, 126, 126–127, 130, 138, 140, 140, 140–141, 144, 153, 155, 155, 155–156, 160, 160, 160–161, 161, 161, 163, 174–175, 184–185, 188, 190, 192–193, 197, 210–211, 218–219, 222, 224–225, 227, 227, 227, 229, 229, 229–230, 230, 230–231, 235–236, 236, 236, 238–239, 239, 239–240, 245, 251, 256–258, 263, 271, 273, 273, 273–274, 277, 285, 287, 287, 287–288, 291, 297, 299, 299, 299–300, 303, 317, 320, 324, 324–325, 334, 347–350, 350, 357–358, 358, 365–367, 385–386, 423–424, 45–46, 59–60, 73–74
charts/core/src/Echart/utils
   updateUtils.ts97.06%90%100%100%26
charts/core/src/EventMarkers/BaseEventMarker
   BaseEventMarker.tsx0%0%0%0%18, 23–24, 27, 27, 27, 31–32, 32, 32, 40, 44–45, 60
   utils.ts0%0%0%0%102, 102, 115, 16, 33–34, 46, 51, 51, 55, 55, 60, 60, 74, 80, 80, 80–81
charts/core/src/EventMarkers/EventMarkerLine
   EventMarkerLine.tsx0%0%0%0%11, 13
charts/core/src/EventMarkers/EventMarkerPoint
   EventMarkerPoint.tsx0%0%0%0%11, 13
charts/core/src/Series
   Series.tsx0%0%0%0%23–26, 26, 26–27, 29–30, 30, 30, 32, 32, 32–34, 41, 44, 49, 63, 66
charts/core/src/Series/Bar
   Bar.tsx0%0%0%0%18, 18, 18, 18, 20, 22, 24, 42, 46, 48, 51, 64, 67, 9
charts/core/src/Series/Line
   Line.tsx0%100%0%0%12, 35–36, 44
charts/core/src/ThresholdLine
   ThresholdLine.tsx0%0%0%0%101, 103–104, 104, 104, 112, 114–115, 119, 30, 60, 60, 98–99
charts/core/src/testUtils
   makeSeriesData.testUtils.ts0%0%0%0%10, 106–107, 109, 111, 114–115, 117–118, 125, 127, 127, 138, 142–143, 146, 149, 15, 151–152, 16, 167, 17, 170–172, 175–176, 182, 185, 187–188, 192, 20, 200, 203, 206, 23, 34, 37, 39, 50, 54, 60, 66–68, 70, 76–77, 80, 83,

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.

3 participants