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
DateTime: Create TimeInput component and integrate into TimePicker #60613
base: trunk
Are you sure you want to change the base?
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @bogiii! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
With a new TimeInput component, the hours value is in 24 hours format.
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @bogiii. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
Thank you for your patience here! Nice refactor, and thank you for adding tests. I did a first-pass review and wrote some comments.
Have you looked into the e2e failures? They seem relevant.
hours={ Number( hours ) } | ||
minutes={ Number( minutes ) } |
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.
Any reason why this is split into two props?
I think it would be more straightforward if it was one value
prop that matched the first argument of the onChange
callback. Most components in this package follow that pattern.
We don't need to worry about re-renders because this is tiny, and most consumers won't stabilize their onChange
functions anyway.
docs: { canvas: { sourceState: 'shown' } }, | ||
}, | ||
args: { | ||
is12Hour: true, |
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 generally want the default story to reflect the component defaults, so let's remove this one. We can show that state in another story if you want to highlight it (no strong opinion).
is12Hour, | ||
hours: entryHours = new Date().getHours(), | ||
minutes: entryMinutes = new Date().getMinutes(), | ||
minutesStep = 1, |
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.
Is this step
prop a strong requirement? If consumers want to override props on the underlying elements, it can be unsustainable to keep adding ad hoc props to the root like this.
I think it's something to triage/address as a separate issue.
<TimeInput hours={ 0 } minutes={ 0 } onChange={ onChangeSpy } /> | ||
); | ||
|
||
const hoursInput = screen.getByLabelText( 'Hours' ); |
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 generally want to use *ByRole()
queries wherever possible, since it can also surface accessibility issues.
const amButton = screen.getByText( 'AM' ); | ||
const pmButton = screen.getByText( 'PM' ); | ||
|
||
expect( amButton ).toHaveClass( 'is-primary' ); |
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 generally want to assert these states through the accessibility tree rather than through styles. And through this test I noticed that this switcher is not screen reader accessible, so I posted an issue here #61163.
This is a pre-existing issue so we don't have to fix it in this PR, but let's maybe add a TODO
comment here to flag that it should be fixed.
onChange: ( obj ) => { | ||
action( 'onChange' )( obj ); | ||
}, |
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.
onChange: ( obj ) => { | |
action( 'onChange' )( obj ); | |
}, | |
onChange: action( 'onChange' ), |
Equivalent?
argTypes: { | ||
onChange: { action: 'onChange', control: { type: null } }, | ||
}, |
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.
argTypes: { | |
onChange: { action: 'onChange', control: { type: null } }, | |
}, | |
argTypes: { | |
onChange: { action: 'onChange', control: { type: null } }, | |
value: { control: { type: null } }, | |
}, |
We've usually been null
-ing out the Storybook control for the value
prop. In this case I think it's especially good because when the value
control can take arbitrary user-generated values, any out-of-bounds value (let's say 100
) can be rendered directly into the input field for the component, and that's weird 😅 And it's overkill to add value-clamping logic before it hits the input's own min/max validation.
What?
TimeInput
componentTimeInput
intoTimePicker
componentWhy?
Before these changes, there was no option to use the simple time input component.
How?
Here are more details: #52734
Testing Instructions
http://localhost:50240/?path=/docs/components-timeinput--docs
--
http://localhost:50240/?path=/docs/components-timepicker--docs
Unit tests: Good old unit tests found a bug, which is resolved. ✅
Testing Instructions for Keyboard
Screenshots or screencast