change format date view #2640
change format date view #2640
Conversation
|
@lukebp ready for review |
There was a problem hiding this comment.
UX tACK on firefox.
Needs code approval from @tiagoalvesdulce.
There was a problem hiding this comment.
Hey vibros, thanks for the PR. Looking good and working as expected. Left some minor inline suggestions. Would be nice if you add some tests for the formatDateView method.
Also, I'm not a big fan of formatDateView naming... It doesn't give much information about what this method does, and how can it be used. Maybe something like getInternationalDateString as it is described on #2634.
PS: You pointed your PR to the wrong issue number 😅
| @@ -47,20 +48,21 @@ const DatePickerField = ({ | |||
| }; | |||
|
|
|||
| const formattedValue = () => { | |||
| console.log(value); | |||
src/helpers.js
Outdated
| @@ -619,3 +620,8 @@ export const calculateAuthorUpdateTree = (authorUpdateId, comments) => { | |||
| } | |||
| return allTreeComments; | |||
| }; | |||
|
|
|||
| export const formatDateView = (value) => { | |||
There was a problem hiding this comment.
can we add unit tests for this helper? Please make sure this method works for falsy values (undefined, null...). What if value has no day prop? And what if no month prop? Will the retuned string behave correctly if an unexpected argument is passed?
IMO, value is not a good name. Maybe you can use prop destruction to make the arguments more readable and control the default values.
| export const formatDateView = (value) => { | |
| export const formatDateView = ({ day, month, year }) => { |
Also, could you please add a more descriptive documentation for this method? Like the ones we already have on our src/helpers for other methods?
src/helpers.js
Outdated
| @@ -619,3 +620,8 @@ export const calculateAuthorUpdateTree = (authorUpdateId, comments) => { | |||
| } | |||
| return allTreeComments; | |||
| }; | |||
|
|
|||
| export const formatDateView = (value) => { | |||
| const dayView = ("0" + value.day).slice(-2); | |||
There was a problem hiding this comment.
| const dayView = ("0" + value.day).slice(-2); | |
| const dayView = `0${value.day}`.slice(-2); |
template strings 😁
|
Needs approval from @tiagoalvesdulce. |
src/helpers.js
Outdated
| * @returns {string} | ||
| */ | ||
| export const getInternationalDateString = ({ day, month, year }) => { | ||
| if (day >= 1 && day <= 31 && month >= 1 && month <= 12 && year >= 1) { |
There was a problem hiding this comment.
This isn't a good way to validate date formats. You are not accounting leap years, that February doesn't have more than 29 days, etc. This should be fine because we can assume startDate, endDate as well as the dates from the DatePicker are always valid. But poor validation is confusing and we can improve it.
There was a problem hiding this comment.
Yes. I have been thinking about this problem. May be we should avoid to convert the date value from the inconsistent values of day, month and year and use the timestamp as the input of the function. How do you think @tiagoalvesdulce ?
There was a problem hiding this comment.
I think that's overkill, we would have to convert the date object 2 times. You can:
- Rename this function to
formatDateToInternationalString.Formatis better thangetbecause you are just formatting the date the way you would like to show it. - Do not validate
dayandyear. Remember, you are just formatting what the user wants to see in the screen. - Validate month because you are using MONTHS_LABELS so you don't want to access an out-of-range index and crash the application.
- This function is only to show the day, year and month in the desired format. All values should've been validated before (in the DatePicker or in the BE).
- Explain why you are not validating the values in the function documentation.
As an alternative, if you want to do the values validation, do it completely, like: https://stackoverflow.com/a/6178341/8903047
Closes #2634
This diff change the date view format of proposal from
9/8/2021to08 Sep 2021