-
Notifications
You must be signed in to change notification settings - Fork 147
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
feat: add dateformat and placeholder prop for datepicker input [DSSUP-199] #2472
feat: add dateformat and placeholder prop for datepicker input [DSSUP-199] #2472
Conversation
🦋 Changeset detectedLatest commit: 107e9d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ PR title follows Conventional Commits specification. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 107e9d7:
|
@@ -92,12 +92,13 @@ const _DatePickerInput = ( | |||
successText, | |||
errorText, | |||
helpText, | |||
dateFormat, | |||
placeholder, |
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.
Its fine to not expose placeholder right? dateFormat can have its own defined placeholder I'm guessing 🤔
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.
I think we should expose the placeholder.
If someone wants to change it from "DD/MM/YYYY" to something like "Select Date," it should be allowed.
developers may want to modify the placeholder to suit their needs.
We should allow developers to modify the placeholder because having a predefined one could impose unnecessary constraints.
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.
Yeah fine. just keep the default as it was before.
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.
If we set placeholder to "Select Date" how would the end-user know which format they are supposed to enter date in 🤔
Works to have placeholder for now since we'll have good default anyways. Can discuss it later.
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.
okay leaving placeholder for now. with default value DD/MM/YYYY.
/** | ||
* Sets the date format to be displayed in the input field. | ||
*/ | ||
dateFormat?: string; |
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.
can we have more explit types like 'DD/MM/YYYY' | 'MMM'
etc?
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 can expose limited options here in dateFormat, but I think using a string is more ideal. What if the user wants to display the date like 'yyyy/mm/dd'? Using a string here allows more flexibility in how the date is formatted. Let me know if we should define an explicit type.
There is one thing, though maybe I should change the default type based on the picker prop as well.
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.
But in this case the consumer wouldn't get autocomplete on which formats are possible no? they'll have to read documentation for that.
What if the user wants to display the date like 'yyyy/mm/dd'?
Also, this is something that we should avoid exposing. If one datepicker on one product is dd/mm/yyyy, other is mm/dd/yyyy, and other is yyyy/mm/dd the consumer will have different experiences of typing dates in all products so this is something that we can put guardrails on.
IMO we can support dd/mm/yyyy | mmm | mmmm | yyyy
. This should cover most usecases while keeping the experience consistent.
There is one thing, though maybe I should change the default type based on the picker prop as well.
We can if we want but its fine even if we skip this since I don't see anyone using day picker just to show month. That experience will look visibly broken so consumers won't do it most likely
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.
added explit formats
return format; | ||
} | ||
if (picker === 'day') { | ||
return 'DD'; |
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.
@anuraghazra @tewarig Shouldn't day picker also be dd/mm/yyyy
by default? just the date number won't be useful in most scenarios no?
Like I would normally use day picker when I want people to only choose a day of given month but I would still want user to know which month they've chosen right?
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.
okay, will change this
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.
changed
5c2e8bb
to
87bd14e
Compare
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.
Mostly LGTM
2ce7b18
<Link | ||
onClick={() => { | ||
onLevelChange('year'); | ||
}} |
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.
need to change to this
Description
The dateFormat prop allows custom formatting for the date displayed in the input.
The placeholder prop enables customization of the placeholder text, which defaults to the date format. This is useful since the placeholder text might differ from the date format or could be unclear to users.
Additional Information
this fixes -
#2469
#2470
Component Checklist