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

fix/GH-70 upcoming events page issue #71

Merged

Conversation

FaberVitale
Copy link
Collaborator

@FaberVitale FaberVitale commented Mar 9, 2024

Description

Fixes #70

Other changes

  • bumps node to 20.11.1.
  • bumps pnpm to latest.
  • updated readme.
  • Use nvmrc to set node version in gh actions.
  • Small overflow issues on mobile portrait.
  • Added vitest as test runner (tests run on ci.yaml)

@FaberVitale FaberVitale requested a review from gdorsi March 9, 2024 10:24
Copy link

netlify bot commented Mar 9, 2024

Deploy Preview for romajs ready!

Name Link
🔨 Latest commit bcc8c59
🔍 Latest deploy log https://app.netlify.com/sites/romajs/deploys/65ecae4a7d350c0008fca5a8
😎 Deploy Preview https://deploy-preview-71--romajs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@gdorsi gdorsi left a comment

Choose a reason for hiding this comment

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

Awesome job!

Screenshot_2024-03-09-16-08-50-50_40deb401b9ffe8e1df2f1cc5ba480b12.jpg

Thanks!

.github/workflows/ci.yaml Show resolved Hide resolved
README.md Show resolved Hide resolved
return (scheduledEvents as UpcomingEvent[])
.concat(placeholderEvents)
.sort((a, b) => a.epochTimeMs - b.epochTimeMs);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the logic is starting to be complex enough to justify some tests.

Do you have opinions about it?

If you agree I can start by creating an issue

Copy link
Collaborator Author

@FaberVitale FaberVitale Mar 9, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

</li>
)}
</Show>
</>
Copy link
Contributor

Choose a reason for hiding this comment

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

This component is becoming a bit hard to read.

How do you feel about splitting in multiple components?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in feb1a5c

@FaberVitale FaberVitale merged commit 893cb0a into Roma-JS:main Mar 23, 2024
5 checks passed
@FaberVitale FaberVitale mentioned this pull request Mar 23, 2024
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.

Upcoming events page doesn't always show next event
2 participants