-
Notifications
You must be signed in to change notification settings - Fork 137
DT-4133 Migrate from momentjs to luxon #5403
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
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.
In this component we are creating DateTime objects and mostly formatting them as strings for comparisons. It's debatable if it would be more robust to compare the objects directly.
looks good to me. Anyway, maybe we should wait until the next UI release (next wednesday) before merging this many changes. Summer break gives us plenty of time to test the changes in dev. |
app/component/ParkAndRideContent.js
Outdated
@@ -80,8 +80,12 @@ function ParkAndRideContent( | |||
if (to - from - 60 * 60 * 24 === 0) { | |||
return [`24${intl.formatMessage({ id: 'hour-short' })}`]; | |||
} | |||
const formattedFrom = moment.utc(from * 1000).format('HH:mm'); | |||
const formattedTo = moment.utc(to * 1000).format('HH:mm'); | |||
const formattedFrom = DateTime.fromMillis(from * 1000) |
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.
DateTime.fromSecond
app/component/ParkAndRideContent.js
Outdated
const formattedFrom = DateTime.fromMillis(from * 1000) | ||
.toUTC() | ||
.toFormat('HH:mm'); | ||
const formattedTo = DateTime.fromMillis(to * 1000) |
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.
fromSeconds
app/component/ParkAndRideContent.js
Outdated
const from = moment.utc(timeSpans.from * 1000).format('HH:mm'); | ||
const to = moment.utc(timeSpans.to * 1000).format('HH:mm'); | ||
const day = date.toLocaleString(currentLanguage, { weekday: 'short' }); | ||
const from = DateTime.fromMillis(timeSpans.from * 1000) |
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.
fromSeconds
app/component/ParkAndRideContent.js
Outdated
const from = DateTime.fromMillis(timeSpans.from * 1000) | ||
.toUTC() | ||
.toFormat('HH:mm'); | ||
const to = DateTime.fromMillis(timeSpans.to * 1000) |
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.
fromSeconds
app/constants.js
Outdated
@@ -66,6 +66,7 @@ export const RealtimeStateType = Object.freeze({ | |||
* This is the date format string to use for querying dates from OTP. | |||
*/ | |||
export const DATE_FORMAT = 'YYYYMMDD'; | |||
export const DATE_FORMAT_LUXON = 'yyyyLLdd'; |
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 bare string 'yyyyLLdd' appears in several places, but they could all use the named constant.
app/util/mqttClient.js
Outdated
@@ -97,7 +97,7 @@ export function parseMessage(topic, message, defaultFeedId) { | |||
operatingDay: | |||
parsedMessage.oday && parsedMessage.oday !== 'XXX' | |||
? parsedMessage.oday | |||
: moment().format('YYYY-MM-DD'), | |||
: DateTime.now().toFormat('yyyy-LL-dd'), |
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.
Actually ALL the date formatting constants could be named and collected in a single place.
Proposed Changes
Pull Request Check List
Review