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

refactor: Events and events.json #88

Merged
merged 13 commits into from
Jan 8, 2024
Merged

refactor: Events and events.json #88

merged 13 commits into from
Jan 8, 2024

Conversation

KevinWu098
Copy link
Member

@KevinWu098 KevinWu098 commented Oct 12, 2023

Summary

  1. Refactored the events page and cleaned up the HTML for consistency and edge cases
  2. Simplified / converted manual updating of events from current to past with filtering
  3. Times should now be provided as an ISO time string (ex: 2023-10-13)
  4. ISO Strings are first compared (for past/upcoming), then converted with toLocaleDateString() for the UI

Page Preview

Screenshot 2023-10-12 at 2 31 04 AM

Test Plan

  1. New events, of the correct ISO time format, should properly display under upcoming events.
  2. Past events, of either correct time format or not, should properly display under past events.
  3. The modals and all related details (look, content, show/hide) should still work as intended

Issues

Closes #87

Future Followup

  1. Handle multi-day events feat: Add support for multi-day events #89
  2. Handle events without images fix: Handle events without Images #94

@ryanmohta
Copy link
Member

Just popping by since this seemed like an interesting PR; I like the idea behind it but have a couple suggestions for the execution. Instead of storing event dates in events.json as formatted strings like Friday, June 23rd, 2023, I would suggest storing them as ISO date strings like 2023-06-23 and formatting them how you want in the front-end using the toLocaleString method and an options object. This would make it super easy to compare event dates with each other and today’s date, and would remove the complexity of creating your own way to obtain the date object from a formatted string.

I also noticed, while looking at events.json, that neither of our methods work for multi-day events like WebJam, so I’m thinking we could get around this by splitting up the time property in each event object into separate startTime and endTime properties, and figure out a good way to represent it in the front-end depending on whether it’s a single or multi-day event. Might be something for a future PR though, since it’s a bit more complex

@KevinWu098
Copy link
Member Author

Just popping by since this seemed like an interesting PR; I like the idea behind it but have a couple suggestions for the execution. Instead of storing event dates in events.json as formatted strings like Friday, June 23rd, 2023, I would suggest storing them as ISO date strings like 2023-06-23 and formatting them how you want in the front-end using the toLocaleString method and an options object. This would make it super easy to compare event dates with each other and today’s date, and would remove the complexity of creating your own way to obtain the date object from a formatted string.

I also noticed, while looking at events.json, that neither of our methods work for multi-day events like WebJam, so I’m thinking we could get around this by splitting up the time property in each event object into separate startTime and endTime properties, and figure out a good way to represent it in the front-end depending on whether it’s a single or multi-day event. Might be something for a future PR though, since it’s a bit more complex

:)) Thanks for the input! Simplifying the event date storage is definitely a smarter implementation, and I'll have that pushed ASAP!

Do agree on the multi-day point (and the scope), so I'll kick that down the road to another Issue/PR!

@KevinWu098 KevinWu098 changed the title refactor: Refactor Events and events.json refactor: Events and events.json Oct 13, 2023
Copy link
Member

@sirAvent sirAvent left a comment

Choose a reason for hiding this comment

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

Just a small change on the Next Image tags!

pages/events.js Outdated Show resolved Hide resolved
Copy link
Member

@sirAvent sirAvent left a comment

Choose a reason for hiding this comment

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

Looks good! Great work on simplifying the data handling!

@KevinWu098 KevinWu098 merged commit a8450b9 into next Jan 8, 2024
1 check passed
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.

Refactor Events Page
3 participants