-
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
User view events #39
User view events #39
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -1,6 +1,14 @@ | |||
/** @type {import('next').NextConfig} */ | |||
const nextConfig = { | |||
reactStrictMode: true, | |||
images: { |
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.
what is this for?
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.
Using Next.js images works with a whitelist of allowed domains, images in test data came from wiki iirc. If we are hosting our own event images on the same domain we can remove this (currently wildcard matches all hosts).
@kevinxiao27 Ok couple things here that weren't in the figma design. We need two views: one for users and one for guests. Users will have registered and saved, whereas guests only see a list of the events. You can can decide what that looks like but I assume that'll just be different tabs for each. The reason why is that guests can also register for events, but they won't have an associated entry in the users table. Secondly, to make it simple, you can just change the "registered" tab to "All events" maybe? or something like that so that there's a tab to view all events. And if the person is registered for an event, it'll just show a "REGISTERED" label instead. What I want to see:
How? Nice animations, good work so far Let me know if you have questions. |
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 Kevin! Great work on the mock-up implementation and animations as always. Props for the quick turnaround with the backend too
I left a similar comment about the Guest view and a few refactoring suggestions for long-term readability
Regarding your notes on the PR:
- I don't see any problem with making the whole card link to the page apart from the save button, as long as the save button is easy to click on mobile. For the clickable save button, could we use shadcn's
<Button>
? I think it would allow a bit of padding to make the icon more clickable - Is the buggy behaviour with AnimatePresence likely? Or a rare edge-case? If it's rare, we can deprioritize finding a fix.
And about the Guest flow:
- It's hard to estimate how many of our registrations go through the Guest flow without asking the Data team, though it's likely a critical path. I'm looking forward to seeing your solution, but also please don't hesitate to message your tentative solutions to the #dev channel. The Guest flow could very well be a problem that other devs should be giving input on given the lack of Figma mock-ups, as it's more of a 'design' decision rather than something that should be figured out over a PR by a single dev
<div className="hidden lg:block">{registeredIndicator(event)}</div> | ||
<div className="hidden lg:block">{timeStateIndicator(event)}</div> |
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.
Do we need these lines? Not sure why they're hidden
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's for the mobile design where these indicators are moved to different parts of the card (get rendered based on breakpoint). There might be a workaround using grid for it to be one-lined but I think I'll keep it like this (or we can simply keep one of the mobile/desktop placements as they don't look too different)
src/pages/events.tsx
Outdated
<div | ||
className={`bg-events-card-bg p-2 h-[46px] rounded-lg flex-row justify-center items-center space-x-1 px-20 shrink hidden lg:flex cursor-pointer ${ | ||
filterState === filterStates.all ? "!bg-events-baby-blue" : "" | ||
}`} | ||
onClick={() => handleUiClick(filterStates.all)} | ||
> | ||
<ListIcon height={20} width={20} className={`${filterState === filterStates.all ? "text-events-user-card-bg fill-current" : ""}`} />{" "} | ||
<p className={`${filterState === filterStates.all ? "text-events-user-card-bg" : ""} text-nowrap`}>All Events</p> | ||
</div> | ||
<div | ||
className={`bg-events-card-bg p-2 h-[46px] rounded-lg flex-row justify-center items-center space-x-1 px-20 shrink hidden lg:flex cursor-pointer ${ | ||
filterState === filterStates.saved ? "!bg-events-baby-blue" : "" | ||
}`} | ||
onClick={() => handleUiClick(filterStates.saved)} | ||
> | ||
<Bookmark height={20} width={20} className={`${filterState === filterStates.saved ? "text-events-user-card-bg fill-current" : ""}`} /> | ||
<p className={`${filterState === filterStates.saved ? "text-events-user-card-bg" : ""}`}>Saved</p> | ||
</div> | ||
</div> | ||
<div className="flex flex-row space-x-3 mb-6 lg:hidden"> | ||
<div | ||
className={`bg-events-card-bg p-2 h-[46px] rounded-lg flex-row justify-center items-center space-x-1 shrink flex grow lg:hidden cursor-pointer ${ | ||
filterState === filterStates.all ? "!bg-events-baby-blue" : "" | ||
}`} | ||
onClick={() => handleUiClick(filterStates.all)} | ||
> | ||
<ListIcon height={20} width={20} className={`${filterState === filterStates.all ? "text-events-user-card-bg fill-current" : ""}`} /> | ||
<p className={`${filterState === filterStates.all ? "text-events-user-card-bg" : ""} text-nowrap`}>All Events</p> | ||
</div> | ||
<div | ||
className={`bg-events-card-bg p-2 h-[46px] rounded-lg flex-row justify-center items-center space-x-1 shrink flex grow lg:hidden cursor-pointer ${ | ||
filterState === filterStates.saved ? "!bg-events-baby-blue" : "" | ||
}`} | ||
onClick={() => handleUiClick(filterStates.saved)} | ||
> | ||
<Bookmark height={20} width={20} className={`${filterState === filterStates.saved ? "text-events-user-card-bg fill-current" : ""}`} /> | ||
<p className={`${filterState === filterStates.saved ? "text-events-user-card-bg" : ""}`}>Saved</p> | ||
</div> | ||
</div> |
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.
Two comments:
- I was having troubles reading this code - at a glance, all of these divs look about the same, with a few minor changes. Would it be possible to improve the readability by an Extract Component refactoring?
- Is the main difference between these divs for responsive design? Could we use Tailwind Breakpoints to simplify this further
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.
Good point! I used composition before as a fast way to build the UI, but extracting components makes sense. I'll also implement breakpoints with flex-basis which should remove some of the redundant code.
Addressed refactoring comments + commented out saved events feature from rendering in the UI, adding a ticket to fix that endpoint to the backlog. gonna work on other tickets now |
src/pages/events.tsx
Outdated
const [email, setEmail] = useState<string>(""); | ||
|
||
useEffect(() => { | ||
console.log(events); |
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.
remove log
Changes:
Issue: #38
Out of Scope (or maybe in scope actually ...):
Notes:
UI Testing
Mobile
mobile.ui.test.mov
Desktop
desktop.ui.test.mov