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
Add event card #22
Add event card #22
Conversation
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.
Great progress 🥳 Some minor comments, you can decide if they need fixing.
margin: 0.25rem 0.25rem 0.25rem 0; | ||
} | ||
|
||
@media screen and (max-width: 600px) { |
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.
It seems that this 600px is used in multiple places. Can it be a variable or constant (I'm not so familiar with modern CSS, so I'm not sure if it is possible 😄 )
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.
Unfortunately, it can't. CSS-variables can't be used in media queries :/ I kinda wish that we could do all without any media queries (at least, for the screen width anyway), but I wanted to just finish these, and maybe I revisit this later and try to find solutions so that we don't need to use these magic numbers 😄
pages/index.tsx
Outdated
|
||
const Home: NextPage = () => { | ||
return ( | ||
<Layout> | ||
<Head> | ||
<title>Alman Akka</title> | ||
<title>Tapahtumat - Alman Akka</title> | ||
<link rel="icon" href="/favicon.ico" /> |
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.
For some reason the browser reports an error not finding the favicon (even though it is visible).
GET http://localhost:3000/favicon.ico 404 (Not Found)
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.
Hey that was a really good catch! The reason is, that the actual favicon is defined in the pages/_document.tsx
, and as it uses an url, I removed the favicon.ico
in the previous PR. So this line here is redundant, I will delete it.
<time dateTime={format(startTimeDate, "yyyy-LL-dd")}> | ||
{format(startTimeDate, shouldShowEndDate ? dayFormatStringWithoutYear : dayFormatString)} | ||
</time> | ||
{shouldShowEndDate ? ( |
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.
This block is a bit unreadable. Consider if splitting it somehow would increase readability.
const startTime = format(startTimeDate, timeFormatString); | ||
const endTime = endTimeDate ? format(endTimeDate, timeFormatString) : null; | ||
|
||
const shouldShowEndDate = !!endTimeDate && !isSameDay(startTimeDate, endTimeDate); |
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.
Could this be written without a double negation?
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.
True, yes, it could.
Purpose
This PR adds some styling to the list of events. I decided to add a bit different layout for the mobile view (see screenshots), as the event card became a bit too busy when the viewport width got smaller.
Oh, and I added
date-fns
for date time editing.Screenshots
Mobile:
Desktop / Tablet:
Related Issues
This PR resolves #8
How to test
Open the front page, and check that all seems to be ok. Also, check in smaller viewport widths as well.