Skip to content
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

Time Picker: Preserve initial input value in Time Picker component #4488

Merged

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jan 30, 2022

Partially addresses #4331

Description

This pull request resolves an issue where if an enhanced Time Picker input has a value, the value is discarded during initialization, thereby preventing a developer from including an initial value for a time picker element.

Additional information

Note that the Fractal component variant was based on a similar Date Picker default value variant, but in that case the default value is expected to be assigned using a data-default-value attribute. Here, I chose instead to let the native value attribute be used since it is likely to be more intuitively understood as a way to control the input value, though I am open to revising this to data-default-value for consistency's sake. Edit: Per feedback, changed to data-default-value in 731748f.


Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

This looks good! I'm leaning towards using data-default-value for consistency. What do you think @thisisdano?

@thisisdano
Copy link
Member

I'm sorry, this one got past me. I agree with James, let's use data-default-value here. We will need a docs update to the Time Picker component to complete this task.

@aduth
Copy link
Contributor Author

aduth commented Apr 6, 2022

Updated to use data-default-value in 731748f.

We will need a docs update to the Time Picker component to complete this task.

Is there anything you need from me to help push this along? In the meantime, I'll remove the "Fixes" auto-close text from the original comment.

@mejiaj
Copy link
Contributor

mejiaj commented Apr 6, 2022

Hey @aduth, thanks for the reminder! Nothing wrong with the functionality, it works well. Just two outstanding issues.

The guidance on Time picker under the Initialization properties the defaults are set on the usa-time-picker element, not the input inside.

Based on that we have these outstanding tasks:

  • Decide if data-default-value be set on usa-time-picker or the input inside (like it currently is in PR). Any preference @thisisdano?
  • Add new settings to docs in time-picker.md

https://github.com/uswds/uswds-site/blob/a4fa3a8bad7706c5e6ac6cf5cf4342d37c36f46d/_components/time-picker/time-picker.md?plain=1#L13-L23

@thisisdano
Copy link
Member

@aduth Sorry for the backtracking, but after reviewing this issue again, we'd like to revert to your initial implementation, using the value attribute on the input. In this instance, respecting native properties and enabling progressive enhancement are more important than consistency between components.

@mejiaj Let's also work to update components like Date Picker to work more like this.

@aduth
Copy link
Contributor Author

aduth commented Apr 8, 2022

Sounds good, pushed the revert in fc6df77.

I still need to find some time to circle back on the documentation to wrap this up.

@thisisdano thisisdano changed the base branch from develop to release-2.13.3 April 11, 2022 03:45
@thisisdano thisisdano merged commit 01af019 into uswds:release-2.13.3 Apr 11, 2022
@thisisdano thisisdano mentioned this pull request Apr 11, 2022
@aduth aduth deleted the aduth-time-picker-initial-value branch April 11, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants