-
Notifications
You must be signed in to change notification settings - Fork 4
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
Episode and next tournament countdown #861
Conversation
39c88f6
to
044f128
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaranmio great work, they look great! My comments were mostly style-focused so please let me know if anything doesn't make sense or if you disagree and we can discuss in the replies to my review comments. 🥇
const thousand = 1000; | ||
const sixty = 60; | ||
const hoursInDay = 24; | ||
const fullPercentage = 100; | ||
const countDownRefreshRate = 30; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we prefer all caps SNAKE_CASE
for constants like these
sqSize: number; | ||
percentage: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be nice to have a set list of options to choose from, like in Spinner.tsx
? 😃
const thousand = 1000; | ||
const sixty = 60; | ||
const hoursInDay = 24; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original constants for these from utils are sufficient and perhaps more clearly specify their purpose (i.e. MILLIS_SECONDS == milliseconds per second)
}); | ||
}, MILLIS_SECOND * SECONDS_MINUTE); | ||
const intervalId = setInterval(() => { | ||
setCount((prevCount) => prevCount + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good! love this implementation
{ProgressCircle({sqSize: progressCircleSize, percentage: percentageDays})} | ||
|
||
<div className={`absolute top-${progressCircleSize / 2} left-${progressCircleSize / 2} top-1/2 start-1/2 transform -translate-x-1/2 -translate-y-1/2 text-center`}> | ||
<span className={`font-bold text-${countTxtSize}`}>{countdownInfo.days}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: looks like we are repeating this classname and others in a few different places... I think it would be good to refactor these into constants that we can reuse and keep them consistent 😎
frontend/src/views/Home.tsx
Outdated
"No upcoming submission deadlines." | ||
)} | ||
<SectionCard title="Next Submission Deadline" loading={nextTournament.isLoading || episode.isLoading}> | ||
{!isNil(episode.data) && new Date().getTime()>episode.data.game_release.getTime() ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can have separate section cards for each of these countdowns? and then we don't need these nested ternaries :)
044f128
to
f36e98a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Added countdown to the next tournament or episode