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

User view events #39

Merged
merged 23 commits into from
Sep 22, 2024
Merged

User view events #39

merged 23 commits into from
Sep 22, 2024

Conversation

kevinxiao27
Copy link

@kevinxiao27 kevinxiao27 commented Aug 20, 2024

Changes:

  • created user events page, events fetched from getStaticProps
  • User specific registrations/saved data from users fetched on client
  • EventDashboard+ EventCard components under EventDashboard; dashboard simply renders events fed in, state handling exists on the page itself. (everything is responsive with tailwind breakpoints)
  • useMemo fed filter function to determine what events should be fed into the dashboard rendering component, has dependencies of ui state, text in search field and saved events.
  • saved/favourited events are optimistically rendered in the UI based on user inputs regardless of the success of PATCH requests being sent to the server.

Issue: #38

Out of Scope (or maybe in scope actually ...):

  • current backend test deployment doesn't have enough data to populate the UI (all events are unpublished, need to mock out user registrations and saved events)
  • backend integration testing

Notes:

  • whole card currently links to the event registration page except clickable save button (might be a ux nightmare), do you guys have any thoughts on what should be made to be clickable vs not
  • known buggy behaviour within AnimatePresence where if you spam child remounts they will not be unmounted properly. I added an artificial click delay on registered and saved buttons to prevent this from happening, but can still occur if you a key/delete repeatedly in the search bar. I confirmed this behaviour didn't have to do with the filtering logic as the events array in useMemo correctly reflects the UI state, but the motion.divs don't unmount. Similar to this but I couldn't find any workarounds on stack overflow
  • mocks come from DynamoDB, faved events field doesnt exist in test but does in prod

UI Testing

Mobile

mobile.ui.test.mov

Desktop

desktop.ui.test.mov
  • (might take a while to look through backend and mock out some events + registrations but the UI functionality is working)
  • will need a follow up for backend integration

Copy link

vercel bot commented Aug 20, 2024

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

Name Status Preview Comments Updated (UTC)
biztech-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 22, 2024 4:36am

@kevinxiao27 kevinxiao27 changed the base branch from main to dev August 20, 2024 07:51
@@ -1,6 +1,14 @@
/** @type {import('next').NextConfig} */
const nextConfig = {
reactStrictMode: true,
images: {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this for?

Copy link
Author

@kevinxiao27 kevinxiao27 Aug 22, 2024

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).

@AllanT102
Copy link
Contributor

AllanT102 commented Aug 23, 2024

@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:

  1. connect it with backend to query actual events
  2. favourite events working
  3. separate views for user and guest

How?
Spin up bt-web and create data/events that will help you test ur different cases. Make sure it is connected to ur local dynamo db instance so that when u use ur bt-webv2 to test, it has that new event data u just put in.
Login with a user to test the view and also access it as a guest. Test out favourite events. I'm not actually too sure if the backend api for fav events works, so do some testing there as well.

Nice animations, good work so far

Let me know if you have questions.

Copy link
Member

@voctory voctory left a 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

src/pages/events.tsx Outdated Show resolved Hide resolved
src/components/EventsDashboard/EventCard.tsx Outdated Show resolved Hide resolved
Comment on lines 112 to 113
<div className="hidden lg:block">{registeredIndicator(event)}</div>
<div className="hidden lg:block">{timeStateIndicator(event)}</div>
Copy link
Member

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

Copy link
Author

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)

Comment on lines 114 to 152
<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>
Copy link
Member

Choose a reason for hiding this comment

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

Two comments:

  1. 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?
  2. Is the main difference between these divs for responsive design? Could we use Tailwind Breakpoints to simplify this further

Copy link
Author

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.

src/pages/events.tsx Outdated Show resolved Hide resolved
src/pages/events.tsx Outdated Show resolved Hide resolved
@kevinxiao27
Copy link
Author

kevinxiao27 commented Aug 29, 2024

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

const [email, setEmail] = useState<string>("");

useEffect(() => {
console.log(events);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove log

@kevinxiao27 kevinxiao27 merged commit 6f982c9 into dev Sep 22, 2024
2 checks passed
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.

4 participants