TimeInput: Expose as subcomponent of TimePicker #63145

mirka · 2024-07-04T19:22:08Z

Follow-up to #60613
Closes #52734

What?

Expose TimeInput as a subcomponent of TimePicker. It can now be used (publicly) as TimePicker.TimeInput.

Why?

So we don't clutter the global namespace when exposing modular parts of a component.

Testing Instructions

The Storybook for TimePicker should be clear about the use of TimePicker.TimeInput.
TimePicker.TimeInput should show a JSDoc in IntelliSense:

Screenshots or screencast

cc @bogiii

github-actions · 2024-07-04T19:22:25Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: bogiii <bogdannikolic@git.wordpress.org>
Co-authored-by: Tropicalista <tropicalista@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

tyxla

Looking good overall.

Glad subcomponents support won't be dropped anytime soon.

I've left a few questions, nothing critical.

tyxla · 2024-07-05T10:33:42Z

packages/components/src/date-time/stories/time.story.tsx

@@ -49,3 +51,16 @@ const Template: StoryFn< typeof TimePicker > = ( {
 };

 export const Default: StoryFn< typeof TimePicker > = Template.bind( {} );
+
+const TimeInputTemplate: StoryFn< typeof TimePicker.TimeInput > = ( args ) => {
+	return <TimePicker.TimeInput { ...args } />;


Felt a bit odd to me that the currentTime doesn't seem to work for the TimeInput story, while it does for the default TimePicker one:

Screen.Recording.2024-07-05.at.13.32.06.mov

You're right, I should've hidden the controls for that story (1bb3f2f).

tyxla · 2024-07-05T10:33:48Z

packages/components/src/date-time/stories/time.story.tsx

@@ -16,6 +16,8 @@ import TimePicker from '../time';
 const meta: Meta< typeof TimePicker > = {
 	title: 'Components/TimePicker',
 	component: TimePicker,
+	// @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170


tyxla · 2024-07-05T10:36:32Z

packages/components/src/date-time/stories/time.story.tsx

@@ -49,3 +51,16 @@ const Template: StoryFn< typeof TimePicker > = ( {
 };

 export const Default: StoryFn< typeof TimePicker > = Template.bind( {} );
+
+const TimeInputTemplate: StoryFn< typeof TimePicker.TimeInput > = ( args ) => {
+	return <TimePicker.TimeInput { ...args } />;


Another thing I spotted in the latest Chrome - there is a subtle visual difference between the colon of the TimePicker and in the solo TimeInput:

TimePicker:

TimeInput:

You'll spot that for the TimeInput in isolation, the colon appears a bit bigger.

Good catch 😳 Fixed in d94947d

tyxla · 2024-07-05T10:43:17Z

packages/components/src/date-time/time/index.tsx

+ * ```
+ */
+TimePicker.TimeInput = TimeInput;
+Object.assign( TimePicker.TimeInput, { displayName: 'TimePicker.TimeInput' } );


We're now exposing TimePicker.TimeInput as a public subcomponent, right? In that case, shouldn't we be documenting it in the README? This reminds me we also don't document TimePicker and DatePicker at this time - do you know if this is intentional?

Correct. I'll be adding a readme in the next PR. I guess there is a bit of inconsistency for subcomponent readmes, but I think TimeInput warrants a separate readme because the props are distinct.

do you know if this is intentional?

It doesn't seem like an oversight, because the DateTimePicker readme does mention that DatePicker and TimePicker can be used individually.

tyxla

LGTM and tests well 👍

One thing I've spotted: the order of components in Storybook is still a bit off:

Could be fixed in a follow-up.

ciampo · 2024-07-08T11:51:55Z

I was going to create an issue where we discuss which naming convention we want to adopt across the components package for "component families" / "compound components" / subcomponents, for better consistency (especially as we plan on having more compound components gong forward).

Do you think it would be beneficial to agree on that naming convention before exposing a new component which may potentially require an export name change soon?

mirka · 2024-07-08T12:41:28Z

the order of components in Storybook is still a bit off

@tyxla Is your expectation that these be alphabetical and not grouped together?

mirka · 2024-07-08T12:43:26Z

Do you think it would be beneficial to agree on that naming convention before exposing a new component which may potentially require an export name change soon?

@ciampo I didn't have any alternative namings for this in mind, but I'm curious what your proposals are. I can wait for your issue to be posted 👍

tyxla · 2024-07-08T12:56:20Z

the order of components in Storybook is still a bit off

@tyxla Is your expectation that these be alphabetical and not grouped together?

Can be either alphabetical or grouped together, but right now they're none of that, and that was unexpected.

ciampo · 2024-07-08T13:01:31Z

Do you think it would be beneficial to agree on that naming convention before exposing a new component which may potentially require an export name change soon?

@ciampo I didn't have any alternative namings for this in mind, but I'm curious what your proposals are. I can wait for your issue to be posted 👍

There it is #63242

# Conflicts: # packages/components/CHANGELOG.md

mirka · 2024-07-31T19:33:49Z

I'm going to merge now since we've agreed on export conventions (#63242).

TimeInput: Make subcomponent of TimePicker

7ecf53b

mirka added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jul 4, 2024

mirka self-assigned this Jul 4, 2024

mirka requested a review from ajitbohra as a code owner July 4, 2024 19:22

mirka mentioned this pull request Jul 4, 2024

DateTime: Create TimeInput component and integrate into TimePicker #60613

Merged

mirka changed the title ~~TimeInput: Make subcomponent of TimePicker~~ Jul 4, 2024

Add changelog

7e872c4

mirka requested a review from a team July 4, 2024 19:24

tyxla reviewed Jul 5, 2024

View reviewed changes

mirka added 2 commits July 6, 2024 02:31

Hide Storybook controls for TimeInput story

1bb3f2f

Add explicit font-size to colon

d94947d

mirka requested a review from tyxla July 5, 2024 17:50

tyxla approved these changes Jul 8, 2024

View reviewed changes

mirka added 2 commits July 30, 2024 21:13

Merge branch 'trunk' into time-input-namespace

27df6fc

# Conflicts: # packages/components/CHANGELOG.md

Merge branch 'trunk' into time-input-namespace

4e659e1

mirka merged commit f0874ac into trunk Jul 31, 2024
61 checks passed

mirka deleted the time-input-namespace branch July 31, 2024 19:34

github-actions bot added this to the Gutenberg 19.0 milestone Jul 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TimeInput: Expose as subcomponent of TimePicker #63145

TimeInput: Expose as subcomponent of TimePicker #63145

mirka commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading

tyxla left a comment

tyxla Jul 5, 2024

mirka Jul 5, 2024

tyxla Jul 5, 2024

tyxla Jul 5, 2024

mirka Jul 5, 2024

tyxla Jul 5, 2024

mirka Jul 5, 2024

tyxla left a comment

ciampo commented Jul 8, 2024

mirka commented Jul 8, 2024

mirka commented Jul 8, 2024 •

edited

Loading

tyxla commented Jul 8, 2024

ciampo commented Jul 8, 2024

mirka commented Jul 31, 2024

TimeInput: Expose as subcomponent of TimePicker #63145

TimeInput: Expose as subcomponent of TimePicker #63145

Conversation

mirka commented Jul 4, 2024 • edited Loading

What?

Why?

Testing Instructions

Screenshots or screencast

github-actions bot commented Jul 4, 2024 • edited Loading

tyxla left a comment

Choose a reason for hiding this comment

tyxla Jul 5, 2024

Choose a reason for hiding this comment

mirka Jul 5, 2024

Choose a reason for hiding this comment

tyxla Jul 5, 2024

Choose a reason for hiding this comment

tyxla Jul 5, 2024

Choose a reason for hiding this comment

mirka Jul 5, 2024

Choose a reason for hiding this comment

tyxla Jul 5, 2024

Choose a reason for hiding this comment

mirka Jul 5, 2024

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

ciampo commented Jul 8, 2024

mirka commented Jul 8, 2024

mirka commented Jul 8, 2024 • edited Loading

tyxla commented Jul 8, 2024

ciampo commented Jul 8, 2024

mirka commented Jul 31, 2024

mirka commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading

mirka commented Jul 8, 2024 •

edited

Loading