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

Event feed initialisation #44

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Event feed initialisation #44

wants to merge 10 commits into from

Conversation

VarMattew
Copy link
Contributor

No description provided.

@VarMattew VarMattew requested review from a team, Tschonti, LeventeFarkashazi, murtagh27 and pem00 and removed request for a team August 19, 2024 11:27
@VarMattew VarMattew linked an issue Aug 19, 2024 that may be closed by this pull request
Copy link

vercel bot commented Aug 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
be-alcoholic-frontend ❌ Failed (Inspect) Sep 23, 2024 4:26pm

Copy link
Member

@Tschonti Tschonti left a comment

Choose a reason for hiding this comment

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

Outstanding work! I left some suggestions or notes for the future, but it looks great!

@@ -0,0 +1,8 @@
export default function EventFeedPage() {
Copy link
Member

Choose a reason for hiding this comment

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

rename the component here

<h2>{e.name}</h2>
<main>
<div className='flex flex-col items-center justify-center bg-green-300'>
<Link href='http://bealcoholic.com'>
Copy link
Member

Choose a reason for hiding this comment

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

don't hardcode any URLs, you can just use '/' here, that will redirect to the main page

<div className='w-[61rem]'>
{events.map((event) => (
<div key={event.id} className='my-4'>
{/* <Link href={`/events/${event.id}`}> */}
Copy link
Member

Choose a reason for hiding this comment

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

why is this commented out?

Copy link
Member

Choose a reason for hiding this comment

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

ohh I see, the other buttons don't work. I'll show you how it can be fixed
spoiler: https://developer.mozilla.org/en-US/docs/Web/API/Event/stopPropagation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you SO MUCH! I spent hours trying to find the answer without any success.

Comment on lines +17 to +22
<Card className='w-48 h-48'>
<CardContent className='flex flex-col aspect-square items-center justify-center p-6 w-auto'>
<span className='text-xl font-semibold no-'>{drinkAction.drink.name}</span>
<span className='text-xs mt-5'>Alcohol Content: {drinkAction.drink.alcoholContent}%</span>
</CardContent>
</Card>
Copy link
Member

Choose a reason for hiding this comment

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

I'd make this smaller

Copy link
Member

Choose a reason for hiding this comment

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

this is sick!

const [copied, setCopied] = useState(false);

const handleCopyClick = (id: string) => {
const linkToCopy = `https://bealcoholic.com/events/${id}`;
Copy link
Member

Choose a reason for hiding this comment

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

we'll store the actual url in an environment variable, but it's fine for now

Comment on lines +61 to +62
<Copy className='h-4 w-4' />
{copied && <span>Copied!</span>}
Copy link
Member

Choose a reason for hiding this comment

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

some spacing would be nice here

return (
<Carousel className='w-[51rem]'>
<CarouselContent className='-ml-1'>
{event.drinkActions.map((drinkAction) => (
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't show every drink action here (though it might work with the carousel), would be better to show some stats, like most consumed drinks

<EventDiscription event={event} />
</div>
<p className='text-gray-600 text-xs mt-2'>
{event.startDate.toLocaleString('hu')} - {event.endDate.toLocaleString('hu')}
Copy link
Member

Choose a reason for hiding this comment

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

this should be further refined, really no need to display the seconds. Would be even better if the common parts of the two dates (year, month and possibly day) would only be shown once

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.

Frontpage (event feed) initial design
2 participants