Skip to content

MPDx - 6916: Remove Date-Fns - #17

Merged
reldredge71 merged 16 commits into
mainfrom
MPDX-6916-Remove-Date-Fns
Feb 26, 2021
Merged

MPDx - 6916: Remove Date-Fns#17
reldredge71 merged 16 commits into
mainfrom
MPDX-6916-Remove-Date-Fns

Conversation

@reldredge71

Copy link
Copy Markdown
Contributor

As a follow-up to adding Luxon to the project, I wanted to rip out Date-Fns. I also wanted to avoid using the default JS Date as much as much as possible -- One of the biggest things about Luxon is that the months are 1-indexed (January = 1, February = 2, etc.) as opposed to JS Date which is 0-indexed (January = 0, February = 1, etc.). I thought this might cause confusion when we are accessing the month property from one library or the other, so I hope this should be consistent and easier to understand.

…-Fns

# Conflicts:
#	src/components/Layouts/Primary/TopBar/NotificationMenu/Item/Item.tsx
#	src/components/Task/Drawer/CommentList/Item/Item.tsx
#	src/components/Task/Drawer/ContactList/Item/Item.tsx
#	src/components/Task/Drawer/Form/Form.mock.tsx
#	src/components/Task/List/List.tsx
#	src/lib/intlFormat/intlFormat.ts
@vercel

vercel Bot commented Feb 23, 2021

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/cru/mpdx-react/HeFsCpEynzErMc8FCM6VMUxadKxH
✅ Preview: https://mpdx-react-git-mpdx-6916-remove-date-fns-cru.vercel.app

@reldredge71

Copy link
Copy Markdown
Contributor Author

There are still some failing tests. They seem to come from Task/Drawer/Form/Form.tsx, Task/Status/Status.tsx, Task/Drawer/Drawer.tsx, and Task/Drawer/CompleteForm/CompleteForm.tsx. I think some of them are failing because of the Mock Material UI Date Picker implementation, but I could use some help diagnosing that. For Status.tsx something seems to be off in the way that the "relative" date format is being displayed, I can look more into that.

Comment thread .storybook/preview.js Outdated
<ThemeProvider theme={theme}>
<CssBaseline />
<MuiPickersUtilsProvider utils={DateFnsUtils}>
<MuiPickersUtilsProvider utils={LuxonUtils} >

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried following these setup instructions, but I wonder if this is connected to the failing tests that seem to use the Material UI date pickers: https://material-ui-pickers.dev/getting-started/installation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OzzieOrca I've stared at this for a while to figure out what Prettier was complaining about, I think it was the extra space after {LuxonUtils}!

@OzzieOrca OzzieOrca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :)

Comment thread package.json
Comment thread src/components/Footer/Footer.test.tsx Outdated
Comment thread src/components/Task/Drawer/CompleteForm/CompleteForm.mock.tsx
Comment thread src/components/Task/Status/Status.tsx Outdated

@OzzieOrca OzzieOrca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except prettier is failing on travis with a storybook preview file.

Comment thread .storybook/preview.js
Comment on lines -18 to -20
if (isChromatic()) {
Settings.now = () => DateTime.local(2020, 2, 1).valueOf();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to figure out what happened to Chromatic and looked into it a little bit and decided to just add this commit to fix 2 things:

  1. With Storybook running locally, the date when making a GraphQL query and the date when creating a mock for that query were seconds/milliseconds off. With the way the GraphQL mocks are set up, this caused the mock not to match the variables of the query and it threw an error that it couldn't find a mock for the query. I decided to just mock the date for now so I removed the isChromatic check to mock the date locally too.
  2. This is the only mock where you used a Luxon function inside the Settings.now function. Apparently that causes an infinite loop 🤷 It must use Settings.now to figure out the date inside which calls it repeatedly. I just used the native date constructor like you used everywhere else.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I did discover that setting Settings.now with Luxon creates an infinite loop. I forgot to take this one out though. Thank you for that!

@reldredge71
reldredge71 merged commit fb305d1 into main Feb 26, 2021
@reldredge71
reldredge71 deleted the MPDX-6916-Remove-Date-Fns branch February 26, 2021 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants