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

DateTime component: Add additional design documentation and image #16757

Merged
merged 4 commits into from
Aug 1, 2019

Conversation

cburton4
Copy link
Contributor

@cburton4 cburton4 commented Jul 25, 2019

Description

Added additional design documentation and best practices for the DateTime component. Also added an image of the component.

@davewhitley davewhitley added [Type] Developer Documentation Documentation for developers Needs Copy Review Needs review of user-facing copy (language, phrasing) Needs Design Feedback Needs general design feedback. labels Jul 25, 2019
@davewhitley davewhitley changed the title Add additional design documentation and image DateTime component: Add additional design documentation and image Jul 25, 2019
@benhuberman
Copy link

benhuberman commented Jul 25, 2019

Hi @cburton4! I'm assuming the copy for review is what's in the Files changed tab, but let me know if I missed something! Here are a few small tweaks I'd propose there:

Before:

a React component to render a calendar and clock for selecting a date and time

After:

a React component that renders a calendar and clock for date and time selection.

Before:

Use smart defaults and highlight that day’s date.

After:

Use smart defaults and highlight the current date.

Before:

Not be used to enter a date that is many years in the future or the past.

After:

Prevent the ability to enter dates in the distant future or past.

For the last point, if there's a consensus around what qualifies as "many years" or "distant" (One year? Three? Ten?) that would be a lot more useful, I think, as people's notions of the distant future/past depend on so many personal and cultural variables.

@cburton4
Copy link
Contributor Author

@benhuberman

Thank you for this feedback. Love these tweaks.

For distant future or past I updated to:

Prevent the ability to enter dates more than a year in the future or past.

I wouldn't say there is consensus on this though. Open to any other suggestions here on what makes sense to other folks.

@davewhitley
Copy link
Contributor

davewhitley commented Jul 29, 2019

Not be used to enter a date that is many years in the future or the past.

I could be wrong, but I don't think it's about preventing that. I interpreted that line like the component just isn't designed to enter a date that is far in the future/past. For example, in order to select a date far into the future/past you would have to click a lot to get to the year you need. The better alternative is to enter the date into a text input field. Although, this component does have text input fields as a secondary way to select a date so maybe this guideline is irrelevant.

@cburton4
Copy link
Contributor Author

Not be used to enter a date that is many years in the future or the past.

I could be wrong, but I don't think it's about preventing that. I interpreted that line like the component just isn't designed to enter a date that is far in the future/past. For example, in order to select a date far into the future/past you would have to click a lot to get to the year you need. The better alternative is to enter the date into a text input field. Although, this component does have text input fields as a secondary way to select a date so maybe this guideline is irrelevant.

Yep, I think you're right. I'll remove this guideline.


Date pickers should:

- Use smart defaults and highlight the current date.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a behavior that is already absorbed by the component itself? Is it worth adding to the docs? A user might ask "how do I do that" since it's mentioned in the "best practices"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see where you're coming from. The idea behind documenting this is to help define the best practices for a component like this in terms of functionality. I think it's useful in case folks in the future want to make an update to this component that might affect this functionality as well as a precedent in case future folks want to make a new component that has similar functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Copy Review Needs review of user-facing copy (language, phrasing) Needs Design Feedback Needs general design feedback. [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants