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

Replace moment with date fns #973 #1035

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

caveman0612
Copy link
Collaborator

Went through and removed all moment, moment-timezone, and react moment references and exchanged them with a matching date-fns or vanilla JS date functions. Was able to consolidate all date functions in the datetime.js util file, and added some basic unit test file for some of the date time functions

Closes #973

@PeterBreen PeterBreen self-assigned this Sep 7, 2023
@PeterBreen PeterBreen temporarily deployed to democracy-lab-staging September 12, 2023 17:53 Inactive
Copy link
Collaborator

@PeterBreen PeterBreen left a comment

Choose a reason for hiding this comment

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

I found two issues in my initial testing. This is deployed on our staging environment (since it happened to be available) so that's where I was doing this: https://democracy-lab-staging.herokuapp.com

First, there's something odd going on with the Create Event datetime.

I put in these dates into the datepicker:
in-form

But when I finish the create event workflow, it previews with these times shown:
in-preview

I noticed that it was September 9, aka 9/9, which seemed odd, so I tested a date in October (I put in Oct 30) and it previewed as 10/10. Looks like somewhere in here the day number is being overridden by the month number.

Second, if I create a new project and then immediately look at the project page, the "Less than a minute ago" datestamp is missing the word Less:
image

(when enough time passes so it reads 'a minute ago', or '14 minutes ago', or whatever - those seem to be fine)

the events and projects I used to test, in case those are relevant:
https://democracy-lab-staging.herokuapp.com/projects/1168
https://democracy-lab-staging.herokuapp.com/events/101
https://democracy-lab-staging.herokuapp.com/events/102

@PeterBreen PeterBreen removed their assignment Sep 13, 2023
@caveman0612
Copy link
Collaborator Author

@PeterBreen I think I was able to fix those two issues you found. the first was just a formatting error and the second was a string parsing mistake. I just removed the parsing to allow for the less than and greater then to show up all the time. Let me know if you want me to change anything else. thanks

@PeterBreen PeterBreen temporarily deployed to democracy-lab-staging September 19, 2023 00:00 Inactive
@PeterBreen
Copy link
Collaborator

@PeterBreen I think I was able to fix those two issues you found. the first was just a formatting error and the second was a string parsing mistake. I just removed the parsing to allow for the less than and greater then to show up all the time. Let me know if you want me to change anything else. thanks

It looks good. I redeployed to staging if you want to check anything yourself, but the project/events I used as test cases are now displaying correctly and the last updated says "less than a minute ago" immediately post-edit.

I haven't done a detailed bundle/loadtime analysis but even a quick pass with browser devtools suggests a notable decrease in bundle size/increase in load speed. 💯

PeterBreen
PeterBreen previously approved these changes Sep 19, 2023
@caveman0612
Copy link
Collaborator Author

@PeterBreen did a quick run through and that the find event page was broken. Seems to be the date is not valid input, but looking into it it seems its trying to format it into the same style as it already is so I just removed the formatting and passed it in straight. Super weird. I will wait on merging until you approve, just to make sure I don't break production.

@PeterBreen PeterBreen temporarily deployed to democracy-lab-staging September 19, 2023 20:18 Inactive
@PeterBreen
Copy link
Collaborator

PeterBreen commented Sep 19, 2023

That is odd, but thanks for catching it. I'm wondering if it's expecting a parse or format somewhere in here? Date always causes trouble...

I'll test this and re-review, but it'll be at least a day or two -- couple other things on the to-do I need to clear first.

One thing that's immediately apparent - it's not sorting past events for me. Compare staging's events page to dev or prod-mirror to see what I mean.

everything in "Past Events" should be in reverse chronological. Current events sort the opposite way (so the closest upcoming event is at the top of the upcoming event list) but there aren't any of those on staging right now.

@caveman0612
Copy link
Collaborator Author

That is super weird. its not doing that in my local build. Let me look into it and see what's going on.

@caveman0612
Copy link
Collaborator Author

This might be a dumb question, but how do you make events? I can make groups and projects, but I can't find a way to make events? Do I need to be signed in to a particular user?

@PeterBreen
Copy link
Collaborator

No, it's not a dumb question - create events aren't accessible the same way groups/projects are. It's a bit of an involved process - I'll send you the details on slack.

@caveman0612
Copy link
Collaborator Author

caveman0612 commented Sep 25, 2023

Thanks for the help with making events. I made some in my local and ran into the same sorting issue. I think I solved it. the datekey was an invalid input for the Date constructor, but it ended up working for the date function, so I kind of nested them lol. If you know a better way to do that please let me know, but I think its all fixed now.

@caveman0612 caveman0612 self-assigned this Sep 28, 2023
@PeterBreen
Copy link
Collaborator

If that's a pattern we might reuse, putting it into a utility function might be nice, otherwise I don't have a problem with it as is.

One other thing, I see a bunch of comments on the import statements reading //checked - do we still need those, or are they just dev notes?

@caveman0612
Copy link
Collaborator Author

yeah the checked was just for me when I was checking over all my code. I just removed them all.

@marlonkeating marlonkeating temporarily deployed to democracy-lab-prod-mirror October 29, 2023 16:59 Inactive
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.

Replace moment.js with date-fns
3 participants