-
Notifications
You must be signed in to change notification settings - Fork 2
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
Convert moment to luxon #4
base: master
Are you sure you want to change the base?
Conversation
src/TextHelpers.js
Outdated
@@ -186,5 +194,5 @@ export const formatDatePair = (startTime, endTime, returnAsObj) => { | |||
} | |||
|
|||
export function isDateInTheFuture (date) { | |||
return moment(date).isAfter(moment()) | |||
return DateTime.fromMillis(date).isAfter(DateTime.now()) |
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.
This function previously was able to accept Dates in many different formats, but is now ONLY capable of accepting timestamps.
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.
there's no way to create a DateTime from different formats? lets just make sure this isn't a problem where we use it of course
const d2 = moment.tz(d1, 'Etc/GMT').hour(21) | ||
const d3 = moment.tz(d2, 'Etc/GMT').day(2) | ||
const d4 = moment.tz(d3, 'Etc/GMT').month(2) | ||
const d5 = moment.tz(d3, 'Etc/GMT').year(2050) |
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.
This way of using the tz function isn't directly replicable with luxon.
@@ -1,11 +1,17 @@ | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|||
exports[`formatDatePair displays differences of dates 1`] = `"Mon, Feb 4, 2019 at 11:41AM MST"`; | |||
exports[`formatDatePair displays differences of dates 1`] = `"Mon, Feb 4, 2019, 11:41 AM CST"`; |
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 lose the "at", but we gain internationalization.
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.
maybe we use @ instead of comma?
No description provided.