Conversation
yet to make minor changes
should be somewhat done
GauravKano
left a comment
GauravKano
left a comment
There was a problem hiding this comment.
Overall, the changes seem great, however, there are a few fixes that need to be made before it can be approved.
| days: 0, | ||
| hours: 0, | ||
| mins: 0, | ||
| secs: 0, |
There was a problem hiding this comment.
We shouldn't be keeping track of the seconds and updating the state every second since the seconds don't need to be displayed
| </h2> | ||
|
|
||
| <p className="font-montserrat text-2xl sm:text-2xl lg:text-3xl pt-5 opacity-90"> | ||
| {event.time} |
There was a problem hiding this comment.
Instead of event.time, we should be displaying event.date in the format of "Day, Month Date" (ex: "Friday, October 18th")
| }; | ||
|
|
||
| updateCountdown(); | ||
| const timer = setInterval(updateCountdown, 1000); |
There was a problem hiding this comment.
We should change this since we do not want to be updating seconds.
On render, we should run the updateCountdown function (which already happens). Then we should run it the second time when the next minute hits (60000 - (Date.now() % 60000)). This can be done with a setTimeout that should also be cleaned up inside the return statement. Then we should run it similar to how we currently have, but with a 60 second interval.
| ); | ||
| } | ||
|
|
||
| const animationClass = isAnimating |
There was a problem hiding this comment.
Could we possibly add different animations for going left and then going right in the carousel?
Not too important, but would be a great change
| onTouchEnd={onTouchEnd} | ||
| > | ||
| {/* Desktop */} | ||
| <div className="hidden w-full items-center justify-center gap-3 sm:flex"> |
There was a problem hiding this comment.
We should most likely change from desktop to mobile at breakpoint md rather than sm
| return ( | ||
| <section className="w-full py-6"> | ||
| <div | ||
| className="mx-auto flex w-full max-w-6xl flex-col items-center gap-4 px-3 sm:px-4" |
There was a problem hiding this comment.
max-w-6xl seems to be too large, I would recommend changing it to be max-w-5xl
| <span className="font-bungee text-2xl sm:text-3xl lg:text-4xl leading-none"> | ||
| {String(unit.value).padStart(2, "0")} | ||
| </span> | ||
| <span className="font-inria mt-1 sm:mt-2 lg:mt-2 text-xs sm:text-base lg:text-xl opacity-85 tracking-wide"> |
There was a problem hiding this comment.
I would recommend using gap in the div on line 104, rather than using margin top in this span.
The current spacing causes the elements in the countdown space to be off center vertically
There was a problem hiding this comment.
The sizing of the fonts and spacing seems great for the desktop (larger screens), however, it seems to stay too large as the screen size shrinks. I would recommend tweaking that a little. Could possibly use breakpoints in tailwind or css clamp
|
I believe I have made the necessary changes now |
2 participants
EventCard and EventCarousel components