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
fix: Add timepicker formating following datepicker #894
fix: Add timepicker formating following datepicker #894
Conversation
Deploy preview for fundamental-ngx ready! Built with commit 7b3313f |
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.
So I think there is a little misunderstanding here. We do need to provide a way for developers to implement their own parsing and formatting of time input values, however the documentation and code in this pull request seems to think that means providing a way for developers to change the :
character to something else. And the naming of this character in the code (rangeDelimiter
) is confusing. For date ranges, the hyphen separates two dates. There is no rangeDelimiter for single date selection. The time picker component only handles a single time selection
I think the formatting and parsing code should work, but there is no need for a delimiter since there is no time range selection, and the docs should demonstrate how to format times to something like 12h30m45s
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.
see my previous comment
I removed range delimiter for time parser/format function, the example is also changed to be DDhDDmDDs . |
|
||
|
||
@Injectable() | ||
export class TimeFormatDashes extends TimeFormatParser { |
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.
Rename the class since this isn't dashes anymore - maybe something like 'CustomTimeFormat'?
Otherwise this PR looks pretty good
Please provide a link to the associated issue.
fixes #805
Please provide a brief summary of this pull request.
I added timepicker component formating which is very similar to datepicker's formating, there are default parse/format functions which can be overwritten by providinf other service.
If this is a new feature, have you updated the documentation?
I added 1 example on documentation and 6 unit tests.