-
Notifications
You must be signed in to change notification settings - Fork 0
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 details page #45
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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'm impressed, this looks really good and is well thought out!
The only thing design wise that came to mind is maybe reversing the position of the event when in mobile view, since text can be pretty long (you can use the attribute flex-col-reverse to change the direction of the content)
I also found on other thing, the EventDetails.tsx
, EvenItem.tsx
and EventsTimeline.tsx
are good abstractions, but they should preferably be closer to where they are used, right now they are placed in /student/_components
, but they are only used in /student/events/...
. It would therefore make more sense to create a _components folder in the events folder
) | ||
} | ||
|
||
export default function EventDetails({ |
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.
Tip for reusable components (you don't have to fix this). In order to provide all the capabilities in the components as in native tags such as <div>
what you can do is
// Now when invoking our component we can provide the event in addition
// to ALL other attributes that you can apply to a <div>, very good for
// accessibility etc
function EventDetails(props: React.HTMLAttributes<HTMLDivElement> & {
event: Event
}) {
// "rest" will be all the attributes in the props type that we did not
// extract, in this case everything except "event" and "className"
const { event, className, ...rest } = props
return (
// Here we pass along all the div attributes to the tag
<div className={cn("...", className} {...rest}>
)
}
import { useRouter, useSearchParams } from "next/navigation" | ||
import { useEffect, useState } from "react" | ||
|
||
import "../../globals.css" |
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.
Is there a specific reason for importing the global.css here? It should already be imported in the layout.tsx
file at the top of the project, it should propagate down
Layout.tsx file
https://github.com/armada-ths/armada.nu/blob/main/src/app/layout.tsx
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.
Nope, think I was trying some stuff and just left the import in by accident
import { Page } from "@/components/shared/Page" | ||
import { notFound } from "next/navigation" | ||
import { fetchEvents } from "@/components/shared/hooks/api/useEvents" | ||
|
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.
Here there should be an SEO section, see https://nextjs.org/docs/app/api-reference/functions/generate-metadata
We should at minimum return a title
and a description
for each event page
1747c4e
to
1acbef2
Compare
1acbef2
to
3e80636
Compare
45ecfd7
to
c316681
Compare
Made this a draft pr since not sure if it makes sense to merge until we have some actual events.