-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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(material/datepicker): add getValidDateOrNull
method
#19915
Conversation
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.
Nice, thanks for the cleanup!
@swseverance can you update the API golden by running |
@mmalerba done. Thanks |
cfd0531
to
5800880
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.
The prefix for this commit should be feat(material/datepicker)
* @param obj The object to check. | ||
* @returns The given object if it is both a date instance and valid, otherwise null. | ||
*/ | ||
getValidDateOrNull(obj: any): D | 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.
I believe this should be unknown
instead of any
(unknown
didn't exist back when we first made the DateAdapter
)
@@ -203,6 +203,14 @@ export abstract class DateAdapter<D> { | |||
*/ | |||
abstract invalid(): D; | |||
|
|||
/** |
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.
In addition to @param
and @returns
, this method should have a description, something like
/**
* Given a potential date object, returns that same date object if it is
* a valid date, or `null` if it's not a valid date.
* @param ...
* @returns ...
*/
it('should provide a method to return a valid date or null', () => { | ||
let d = new Date(); | ||
expect(adapter.getValidDateOrNull(d)).toBe(d); | ||
expect(adapter.getValidDateOrNull(new Date(NaN))).toBeNull(); |
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 would probably also add cases for null
, undefined
, ''
, 0
, a date string, and a timestamp just to be comprehensive.
Several components have identical implementations of a `_getValidDateOrNull` method. This PR reduces code duplication by adding a `getValidDateOrNull` method to the `DateAdapter` class for the components to use instead.
5800880
to
2e76765
Compare
@jelbourn I made the requested changes. |
getValidDateOrNull
methodgetValidDateOrNull
method
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.
LGTM
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Several components have identical implementations of a
_getValidDateOrNull
method. This PR reduces code duplication by adding agetValidDateOrNull
to theDateAdapter
class for the components to use instead.