replace moment with helpers based on date-fns #223
Conversation
package.json
Outdated
| ] | ||
| ], | ||
| "dependencies": { | ||
| "date-fns": "^2.28.0" |
There was a problem hiding this comment.
Nitpick: I would move it to devDeps for consistency.
|
|
||
| export default helper(function plainDate([datetime]/*, hash*/) { | ||
| try { | ||
| return lightFormat(datetime, 'dd/MM/yyyy'); |
There was a problem hiding this comment.
In OP we created a helper that uses Intl.Datetimeformat (with hardcoded country since we didn't have other requirements).
Maybe something similar can be done here? Or do you prefer providing the format manually?
There was a problem hiding this comment.
on testing I ended up with a different separator (I think -) and / was preferred here. I can recheck with customer if that's still important
There was a problem hiding this comment.
The helper I linked has the following output:
In any case, it works as intended at the moment so this remark isn't really a blocker.
There was a problem hiding this comment.
Not entirely: formatting as 'dd/MM/yyyy' would give '21/01/2022' and Intl gives '21/1/2022', unless you use Intl.DateTimeFormat("nl-BE", { dateStyle: "short" }).format(datetime);. Weird, but consistent across browsers.
|
|
||
| export default helper(function detailedDate([datetime]/*, hash*/) { | ||
| try { | ||
| return lightFormat(datetime, 'dd/MM/yyyy HH:mm'); |
There was a problem hiding this comment.
I think this can be achieved with Intl.Datetimeformat as well by using some of the options:
console.log(new Intl.DateTimeFormat('nl-BE', { dateStyle: 'short', timeStyle: 'short' }).format(date));
There was more leftover config for the removed dependency "moment" in environment.js.
Moved date-fns to devDependencies instead of regular dependencies.
With an instance check for Date, the helper no longer tries to format things that are not dates, such as undefined or other objects. In some instances, a date is left undefined, such as the end date on meetings if the meeting isn't finished yet. We don't want unnecessary errors being printend on the console because of this.
Completely missed this during testing. Used the wrong reference.
|
|
||
| export default helper(function detailedDate([datetime]/*, hash*/) { | ||
| //If not a date (e.g. date is undefined) return "" for printing on screen. | ||
| if (!(datetime instanceof Date)) return ""; |
There was a problem hiding this comment.
I know it's merged already, but this check seems redundant since the try catch handles it already? And the comment above seems redundant as well since it's literally the transcript of what you think when you read the actual line of code 😄.
There was a problem hiding this comment.
I thought the intention with the try...catch was to print an error in case the actual dates did not make sense, but we don't want an error when the date is just undefined, because that can happen in a normal situation. I wanted error messages only when really necessary to not cause confusion later.
Yes, the comment is pretty useless like this, sorry for that, but at least it means this check was put there for a reason.
visually verified the changes and all seems to be ok, there's some small changes (we were inconsistent and sometimes formatted as
dd-MM-YYYY, now we are consistently using/).