Skip to content

Migrate TimePicker to Floating UI #878#952

Open
benjaminLeongSK wants to merge 6 commits intomasterfrom
BL/878
Open

Migrate TimePicker to Floating UI #878#952
benjaminLeongSK wants to merge 6 commits intomasterfrom
BL/878

Conversation

@benjaminLeongSK
Copy link
Contributor

Changes

  • Migrate time picker to use shared dropdown
  • Added new props
  • Added unit test
  • Changed design

@benjaminLeongSK benjaminLeongSK requested a review from qroll February 5, 2026 09:46
@qroll qroll added the type: enhancement New feature or request label Feb 10, 2026
@qroll qroll linked an issue Feb 10, 2026 that may be closed by this pull request
Comment on lines -82 to -85
${MediaQuery.MaxWidth.lg} {
flex-direction: column;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed?


const handleSelectionDropdownCancel = () => {
runOnBlurHandler();
const open = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to handleOpen and move to EVENT LISTENERS section

const handleChange = (value: string) => {
onChange?.(value);
runOnBlurHandler();
const close = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to handleClose and move to EVENT LISTENERS section

Comment on lines +115 to +116
onClose={() => close()}
onDismiss={() => close()}
Copy link
Contributor

Choose a reason for hiding this comment

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

can just be onClose={handleClose()}?

clickToToggle={false}
offset={8}
alignment={alignment}
fitAvailableHeight
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed?

Comment on lines +99 to +102
onChange={(v) => {
onChange?.(v);
close();
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

can restore the handleChange function and move the required logic there

Copy link
Contributor

Choose a reason for hiding this comment

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

on selection, restore the focus to the InputWrapper

setFloatingRef,
getFloatingProps,
}: DropdownRenderProps) => {
if (!isOpen) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably not needed since ElementWithDropdown already handles the visibility?

export const Label = styled.label<LabelStyleProps & { $selected?: boolean }>`
${(props) =>
props.$selected
? Font["body-baseline-bold"]
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for catching this! looking at Figma, it's meant to be body-baseline-semibold instead

renderDropdown={renderDropdown}
onOpen={open}
onClose={() => close()}
onDismiss={() => close()}
Copy link
Contributor

Choose a reason for hiding this comment

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

for dismiss action, restore the focus back to the InputWrapper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate TimePicker to Floating UI

2 participants

Comments