Skip to content
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

Removes Moment-Timezone and introduces Luxon #2476

Merged
merged 6 commits into from Jun 11, 2019

Conversation

Projects
None yet
5 participants
@kierangillen
Copy link
Contributor

commented May 29, 2019

  • This PR completely removes all references to moment-timezone and replaces it with Luxon as discussed in #2474
  • This PR is the first step in completely removing both moment and moment-timezone but is isolated to moment-timezone for an easier transition.
  • Updates datetime formatting in accordance to our new date formatting guidelines where luxon formatting has been updated - https://www.notion.so/artsy/Formatting-Time-f6f4aaeeba414583a207013fec5a9794
@damassi

This comment has been minimized.

Copy link
Member

commented May 29, 2019

A less extensive 1st-pass moment-to-luxon refactor could also take place in Palette. A while back @dblandin ended up adding moment / moment-timezone and we always meant to follow up as it's increased the bundle-size quite a bit due to all of the timezone code. (Ah, just saw RFC further up notification feed 👍)

@artsyit

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Deploy preview for artsy-reaction ready!

Built with commit ba63cf3

https://deploy-preview-2476--artsy-reaction.netlify.com

@kierangillen kierangillen force-pushed the kierangillen:remove_moment branch from 49313cd to ad37779 Jun 10, 2019

@kierangillen kierangillen force-pushed the kierangillen:remove_moment branch from ad37779 to 4f1c1e4 Jun 10, 2019

@kierangillen kierangillen force-pushed the kierangillen:remove_moment branch from 68b0793 to 04dd6b0 Jun 10, 2019

@peril-staging

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Fails
🚫 Please add a description to your PR.
🚫 @types dependencies were added to package.json, as a dependency for others.
You need to move @types/luxon into "devDependencies"?

New dependencies added: @types/luxon and moment.

@types/luxon

Author: Unknown

Description: TypeScript definitions for luxon

Homepage: http://npmjs.com/package/@types/luxon

Createdover 1 year ago
Last Updated11 days ago
LicenseMIT
Maintainers1
Releases27
Direct Dependencies
README

Installation

npm install --save @types/luxon

Summary

This package contains type definitions for luxon ( https://github.com/moment/luxon#readme ).

Details

Files were exported from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/luxon

Additional Details

  • Last updated: Thu, 30 May 2019 18:05:34 GMT
  • Dependencies: none
  • Global values: none

Credits

These definitions were written by Colby DeHart https://github.com/colbydehart, Hyeonseok Yang https://github.com/FourwingsY, Jonathan Siebern https://github.com/jsiebern, Matt R. Wilson https://github.com/mastermatt, Pietro Vismara https://github.com/pietrovismara, Janeene Beeforth https://github.com/dawnmist, Jason Yu https://github.com/ycmjason, Miklos Danka https://github.com/mdanka.

moment

Author: Iskren Ivov Chernev

Description: Parse, validate, manipulate, and display dates

Homepage: http://momentjs.com

Createdover 7 years ago
Last Updated1 day ago
LicenseMIT
Maintainers5
Releases62
Keywordsmoment, date, time, parse, format, validate, i18n, l10n and ender
README

Join the chat at https://gitter.im/moment/moment

NPM version NPM downloads MIT License Build Status
Coverage Status
FOSSA Status
SemVer compatibility

A lightweight JavaScript date library for parsing, validating, manipulating, and formatting dates.

Documentation

Port to ECMAScript 6 (version 2.10.0)

Moment 2.10.0 does not bring any new features, but the code is now written in
ECMAScript 6 modules and placed inside src/. Previously moment.js, locale/*.js and
test/moment/*.js, test/locale/*.js contained the source of the project. Now
the source is in src/, temporary build (ECMAScript 5) files are placed under
build/umd/ (for running tests during development), and the moment.js and
locale/*.js files are updated only on release.

If you want to use a particular revision of the code, make sure to run
grunt transpile update-index, so moment.js and locales/*.js are synced
with src/*. We might place that in a commit hook in the future.

Upgrading to 2.0.0

There are a number of small backwards incompatible changes with version 2.0.0. See the full descriptions here

  • Changed language ordinal method to return the number + ordinal instead of just the ordinal.

  • Changed two digit year parsing cutoff to match strptime.

  • Removed moment#sod and moment#eod in favor of moment#startOf and moment#endOf.

  • Removed moment.humanizeDuration() in favor of moment.duration().humanize().

  • Removed the lang data objects from the top level namespace.

  • Duplicate Date passed to moment() instead of referencing it.

Changelog

Contributing Open Source Helpers

We're looking for co-maintainers! If you want to become a master of time please
write to ichernev.

In addition to contributing code, you can help to triage issues. This can include reproducing bug reports, or asking for vital information such as version numbers or reproduction instructions. If you would like to start triaging issues, one easy way to get started is to subscribe to moment/moment on CodeTriage.

License

Moment.js is freely distributable under the terms of the MIT license.

FOSSA Status

Generated by 🚫 dangerJS against 68b0793

@kierangillen kierangillen force-pushed the kierangillen:remove_moment branch from 8f86ff8 to 3184257 Jun 10, 2019

@kierangillen kierangillen changed the title WIP: Begins replacing moment-timezone for luxon Removes Moment-Timezone and introduced Luxon Jun 10, 2019

@kierangillen kierangillen changed the title Removes Moment-Timezone and introduced Luxon Removes Moment-Timezone and introduces Luxon Jun 10, 2019

@dblandin
Copy link
Member

left a comment

Is the format change intentional?

For example, "May 19, 2017 9:09 am" to "May 19, 2017 9:09am"?

Also echoing a similar comment in another PR to prefer format strings which would be easier to maintain.

} else {
hour = dateTime.hour
}
const time = `${hour}:${minutes}${amPm}`

This comment has been minimized.

Copy link
@dblandin

dblandin Jun 10, 2019

Member

I'll echo the same comment from https://github.com/artsy/palette/pull/505/files/5c3707b56acd3e0f8ede9a53f9473c6d6041c4da#r292103479

This styling seems simple enough to defer to a format string, rather than constructing it ourselves.

This comment has been minimized.

Copy link
@kierangillen

kierangillen Jun 10, 2019

Author Contributor

Luxon's formatting strings don't provide the display formatting for our styling requirements.

This comment has been minimized.

Copy link
@kierangillen

kierangillen Jun 10, 2019

Author Contributor

Like lowercase am/pm for example.

This comment has been minimized.

Copy link
@damassi

damassi Jun 10, 2019

Member

(In the future these sorts of things should be called out prominantly in the description.)

This comment has been minimized.

Copy link
@dblandin

dblandin Jun 10, 2019

Member

Luxon's formatting strings don't provide the display formatting for our styling requirements.

Like lowercase am/pm for example.

Ah, I didn't realize that! Thanks for clarifying 👍

I found a related issue and a good response to it here: moment/luxon#224 (comment)

Looks like we might be able to use toLocaleParts to customize the casing on the meridiem part. No need to block this PR though.

I'm also not sure where the decision was made to remove the space between minutes and amPm. Could you provide a link to that?

This comment has been minimized.

Copy link
@kierangillen

kierangillen Jun 10, 2019

Author Contributor

All of our time formatting is located in this notion doc https://www.notion.so/artsy/Formatting-Time-f6f4aaeeba414583a207013fec5a9794 @dblandin

@artsy artsy deleted a comment from peril-staging bot Jun 10, 2019

@artsy artsy deleted a comment from peril-staging bot Jun 10, 2019

@artsy artsy deleted a comment from peril-staging bot Jun 10, 2019

@artsy artsy deleted a comment from peril-staging bot Jun 10, 2019

@artsy artsy deleted a comment from peril-staging bot Jun 10, 2019

@@ -37,11 +37,11 @@ describe("Date", () => {
describe("#getDate", () => {
const timestamp = "2017-02-22T19:22:05.709Z"
const expectedFormattedDates = {
monthYear: "February 2017",
monthYear: "Feb 2017",

This comment has been minimized.

Copy link
@zephraph

zephraph Jun 10, 2019

Contributor

Is this actually going to be changing date formats to shorthand? Is that something we expect to see replicated in the UI?

This comment has been minimized.

Copy link
@kierangillen

kierangillen Jun 10, 2019

Author Contributor

Yes it's a recent change to our date formatting - https://www.notion.so/artsy/Formatting-Time-f6f4aaeeba414583a207013fec5a9794

This comment has been minimized.

Copy link
@zephraph

zephraph Jun 10, 2019

Contributor

Ahh, I see. 👍

@dblandin

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@kierangillen This PR seems to be accomplishing two things which is leading to some confusion:

  1. Removing moment-timezone in favor of luxon
  2. Updating date/time formatting to adhere to Artsy Brand Guidelines

What do you think of either:

  1. Splitting these changes into two PRs, with each addressing one of the goals above?
  2. Or updating the description to refer to both goals and linking to relevant Jira tickets / Notion documents?
@kierangillen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@dblandin I updated the description to mention there are changes to the date formatting. The date formatting changes are something that need to be done through all of our repos and are only done in this PR because the code producing the formatting is being touched. Therefore it's not one of the aims of this PR but a side effect.

@@ -41,7 +40,21 @@ export class AuctionTimer extends React.Component<Props> {
}

labelWithTimeRemaining() {
const display = moment(this.endDate).format("MMM D, h:mma")

This comment has been minimized.

Copy link
@zephraph

zephraph Jun 10, 2019

Contributor

If there are many of these more complex formatting tasks it might be good to add a TODO to pull these out into a common datetime formatting file.

I'm okay with that not happening in this PR though.

This comment has been minimized.

Copy link
@kierangillen

kierangillen Jun 10, 2019

Author Contributor

Many of these formats are already in metaphysics which would be the most ideal. However removing moment and also updating all of the relay queries to pull in the correct formatting seamed like two separate tasks :)

@zephraph

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

It's looking pretty good. I'm going to wait for the others to 👍 the review before merging.

@dblandin

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

The date formatting changes are something that need to be done through all of our repos and are only done in this PR because the code producing the formatting is being touched. Therefore it's not one of the aims of this PR but a side effect.

Yep, that's clear to me now! Thanks for clarifying.

In the future, I'd suggest either splitting off drive-by changes to a following PR or making those considerations clear and up-front in the PR description to ease review 👍

Overall, this is great work! Thanks for tackling this! 🎉

@zephraph

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

I'm gonna go ahead and merge 👍

@zephraph zephraph merged commit cda3040 into artsy:master Jun 11, 2019

7 checks passed

ci/circleci: jest Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: relay Your tests passed on CircleCI!
Details
ci/circleci: type-check Your tests passed on CircleCI!
Details
ci/circleci: update-cache Your tests passed on CircleCI!
Details
ci/circleci: workflow-queue Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details
@artsyit

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

🚀 PR was released in v16.16.0 🚀

@artsyit artsyit added the released label Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.