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

[UXIT-1539] Events Page · Schedule #704

Merged
merged 12 commits into from
Oct 11, 2024
Merged

[UXIT-1539] Events Page · Schedule #704

merged 12 commits into from
Oct 11, 2024

Conversation

mirhamasala
Copy link
Collaborator

@mirhamasala mirhamasala commented Oct 9, 2024

📝 Description

This pull request introduces a new feature to handle event schedules, including the addition of a schema for schedules, updates to the event data structure, and UI components to display the schedule on event pages. The most important changes include adding a new schedule schema, updating the event data structure, and creating new components for displaying the schedule.

  • Type: New feature

Note

For the sake of speed, we have not implemented the mobile dropdown at this stage, instead opting for a sticky horizontal scrollbar. With Filipa unavailable, we don’t yet have a solid solution for handling the x-scrollbar effectively. This is also a pending task for the Allocators table.

The mobile dropdown will also be required for the Security Maturity Model Table of Contents, and at that point, we can refactor both implementations.

🛠️ Key Changes

Schema and Data Structure Updates:

  • Added a new ScheduleSchema to define the structure of event schedules, including days and events (src/app/_schemas/event/ScheduleSchema.ts).
  • Updated the EventFrontMatterSchema to include the new schedule field (src/app/_schemas/event/FrontMatterSchema.ts).
  • Modified the convertMarkdownToEventData function to handle the new schedule field (src/app/_utils/convertMarkdownToEventData.ts).

UI Components:

Event Page Integration:

Utility Functions:

Configuration Update:

  • Updated the admin configuration to include the new schedule field (public/admin/config.yml).

📌 To-Do Before Merging

  • Remove test data

🧪 How to Test

📸 Screenshots

CleanShot 2024-10-10 at 10 14 15@2x

Copy link

vercel bot commented Oct 9, 2024

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

Name Status Preview Comments Updated (UTC)
filecoin-foundation-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 11, 2024 0:43am

Copy link

@barbaraperic
Copy link
Collaborator

Maybe we can say something like "No events on this day", or not show the day at all if it doesn't have events?

Screenshot 2024-10-10 at 11 39 01

@barbaraperic
Copy link
Collaborator

On mobile looks good! Tho there's a slight glitch when you scroll down in the first tab and click on the second tab. It doesn't take you back in the view - maybe something we can implement if not too complicated.

RPReplay_Final1728557038.mov

const tabGroupRef = useRef<HTMLDivElement>(null)
const hasMounted = useRef(false)

const validDays = schedule!.days.filter((day) => day.events.length > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've noticed you use "!" in couple of cases here - is there an advantage over "?" ?

Copy link
Collaborator Author

@mirhamasala mirhamasala Oct 10, 2024

Choose a reason for hiding this comment

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

TypeScript thinks that schedule can be undefined but, at this point, we know for sure that schedule isn't undefined since we check for it with eventHasSchedule - and only then render the component.

You can postfix an expression with ! to tell TypeScript that you know it's not null or undefined. This works the same as an 'as' assertion.

Copy link
Collaborator

@CharlyMartin CharlyMartin Oct 11, 2024

Choose a reason for hiding this comment

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

This is not super safe as schedule comes from outside the component. The consumer of the component doesn't know that schedule has to exist, and TypeScript will not complain if they try to pass undefined.

// Works fine
{eventHasSchedule && <ScheduleSection schedule={schedule} />}

// Should fail but doesn't
<ScheduleSection schedule={schedule} />

As safer solution would be to type schedule as non nullable:

type ScheduleTabsProps = {
  schedule: NonNullable<Event['schedule']>
}

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is amazing.

Copy link
Collaborator

@barbaraperic barbaraperic left a comment

Choose a reason for hiding this comment

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

Looking good, well done! Left a few non-blocking comments

const tabGroupRef = useRef<HTMLDivElement>(null)
const hasMounted = useRef(false)

const validDays = schedule!.days.filter((day) => day.events.length > 0)
Copy link
Collaborator

@CharlyMartin CharlyMartin Oct 11, 2024

Choose a reason for hiding this comment

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

This is not super safe as schedule comes from outside the component. The consumer of the component doesn't know that schedule has to exist, and TypeScript will not complain if they try to pass undefined.

// Works fine
{eventHasSchedule && <ScheduleSection schedule={schedule} />}

// Should fail but doesn't
<ScheduleSection schedule={schedule} />

As safer solution would be to type schedule as non nullable:

type ScheduleTabsProps = {
  schedule: NonNullable<Event['schedule']>
}

What do you think?

Comment on lines 16 to 39
type ScheduleTabsProps = {
schedule: Event['schedule']
}

const { screens } = theme

export function ScheduleTabs({ schedule }: ScheduleTabsProps) {
const tabGroupRef = useRef<HTMLDivElement>(null)
const hasMounted = useRef(false)

const validDays = schedule!.days.filter((day) => day.events.length > 0)

useEffect(() => {
hasMounted.current = true
}, [])

function scrollToTabGroup() {
if (hasMounted.current && isScreenBelowLg() && tabGroupRef.current) {
tabGroupRef.current.scrollIntoView({
behavior: 'smooth',
block: 'start',
})
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type ScheduleTabsProps = {
schedule: Event['schedule']
}
const { screens } = theme
export function ScheduleTabs({ schedule }: ScheduleTabsProps) {
const tabGroupRef = useRef<HTMLDivElement>(null)
const hasMounted = useRef(false)
const validDays = schedule!.days.filter((day) => day.events.length > 0)
useEffect(() => {
hasMounted.current = true
}, [])
function scrollToTabGroup() {
if (hasMounted.current && isScreenBelowLg() && tabGroupRef.current) {
tabGroupRef.current.scrollIntoView({
behavior: 'smooth',
block: 'start',
})
}
}
type ScheduleTabsProps = {
schedule: NonNullable<Event['schedule']>
}
const { screens } = theme
export function ScheduleTabs({ schedule }: ScheduleTabsProps) {
const tabGroupRef = useRef<HTMLDivElement>(null)
const hasMounted = useRef(false)
const validDays = schedule.days.filter((day) => day.events.length > 0)
useEffect(() => {
hasMounted.current = true
}, [])
function scrollToTabGroup() {
if (hasMounted.current && isScreenBelowLg() && tabGroupRef.current) {
tabGroupRef.current.scrollIntoView({
behavior: 'smooth',
block: 'start',
})
}
}

Comment on lines 7 to 17
type ScheduleSectionProps = {
schedule: Event['schedule']
}

export function ScheduleSection({ schedule }: ScheduleSectionProps) {
return (
<PageSection kicker="Join Us" title={schedule?.title || 'Schedule'}>
<ScheduleTabs schedule={schedule} />
</PageSection>
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type ScheduleSectionProps = {
schedule: Event['schedule']
}
export function ScheduleSection({ schedule }: ScheduleSectionProps) {
return (
<PageSection kicker="Join Us" title={schedule?.title || 'Schedule'}>
<ScheduleTabs schedule={schedule} />
</PageSection>
)
}
type ScheduleSectionProps = {
schedule: NonNullable<Event['schedule']>
}
export function ScheduleSection({ schedule }: ScheduleSectionProps) {
return (
<PageSection kicker="Join Us" title={schedule.title || 'Schedule'}>
<ScheduleTabs schedule={schedule} />
</PageSection>
)
}

Comment on lines 7 to 28
type SpeakersSectionProps = {
speakers: Event['speakers']
}

export function SpeakersSection({ speakers }: SpeakersSectionProps) {
return (
<PageSection kicker="speakers" title="Speakers">
<CardGrid cols="mdTwo">
{speakers!.map((speaker) => (
<KeyMemberCard
key={speaker.name}
{...speaker}
image={{
...speaker.image,
alt: `Photo of ${speaker.name}`,
}}
/>
))}
</CardGrid>
</PageSection>
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type SpeakersSectionProps = {
speakers: Event['speakers']
}
export function SpeakersSection({ speakers }: SpeakersSectionProps) {
return (
<PageSection kicker="speakers" title="Speakers">
<CardGrid cols="mdTwo">
{speakers!.map((speaker) => (
<KeyMemberCard
key={speaker.name}
{...speaker}
image={{
...speaker.image,
alt: `Photo of ${speaker.name}`,
}}
/>
))}
</CardGrid>
</PageSection>
)
}
type SpeakersSectionProps = {
speakers: NonNullable<Event['speakers']>
}
export function SpeakersSection({ speakers }: SpeakersSectionProps) {
return (
<PageSection kicker="speakers" title="Speakers">
<CardGrid cols="mdTwo">
{speakers.map((speaker) => (
<KeyMemberCard
key={speaker.name}
{...speaker}
image={{
...speaker.image,
alt: `Photo of ${speaker.name}`,
}}
/>
))}
</CardGrid>
</PageSection>
)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahhhh TS Master 🙇🏽‍♀️

Comment on lines 28 to 30
useEffect(() => {
hasMounted.current = true
}, [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice little optimisation - we can import useIsMounted from usehooks-ts, which is already part of the project :)

Comment on lines +96 to +99
function isScreenBelowLg() {
return window.matchMedia(`(max-width: ${parseInt(screens.md, 10) - 1}px)`)
.matches
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already rely on useMediaQuery from usehooks-ts in the project. Maybe it can be useful here as well?

Comment on lines +34 to +37
tabGroupRef.current.scrollIntoView({
behavior: 'smooth',
block: 'start',
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have a function scrollToSection in the maturity model utils folder, in case it can be extracted in the global utils reused here. Not sure, I haven't tried.

Super cool you added this by the way, smooth UX!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Decided not to extract the scrollToSection function for now because it is tightly coupled to the concept of a SectionHash.

By the way, this made me notice that the function scrollToSection uses document.querySelector, which in React is considered an anti-pattern because it directly manipulates the DOM outside of React’s control. Instead, React encourages the use of refs to interact with DOM elements.

Something like

import { useRef } from 'react'

export function scrollToSection(sectionRef: React.RefObject<HTMLElement>) {
  if (!sectionRef.current) {
    console.error(`The referenced element does not exist`)
    return
  }

  sectionRef.current.scrollIntoView({
    block: 'start',
    behavior: 'smooth',
  })
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed!

@CharlyMartin
Copy link
Collaborator

@mirhamasala I added a blocking comment about the prop type of the section components. I believe we should use NonNullable instead of ! to be more accurate / on the safe side. I added more context in the comments, I hope it makes sense.

I also added an event via the CMS, and it worked great. The event I created was added at the end of the list, even though it happened sooner. How about sorting validDays by soonest to furthest?

CleanShot 2024-10-11 at 11 15 21

The page looks really good and professional, it will be great for Bangkok! Good job ✨👏

@mirhamasala
Copy link
Collaborator Author

@CharlyMartin

How about sorting validDays by soonest to furthest?

Oh, good catch! Thanks so much for the review, Charly! It was super helpful.

@mirhamasala mirhamasala merged commit 0d4c834 into main Oct 11, 2024
5 checks passed
@mirhamasala mirhamasala deleted the mm/events-schedule branch October 11, 2024 12:49
@CharlyMartin
Copy link
Collaborator

Oh, good catch! Thanks so much for the review, Charly! It was super helpful.

Anytime 🫡

Copy link

sentry-io bot commented Oct 16, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: RecapSection is not defined /events/fil-bangkok-2024 View Issue
  • ‼️ TypeError: (0 , date_fns_tz__WEBPACK_IMPORTED_MODULE_0_.utcToZonedTime) is not a function /events/fil-bangkok-2024 View Issue
  • ‼️ ReferenceError: utcToZonedTime is not defined /events/fil-bangkok-2024 View Issue
  • ‼️ TypeError: (0 , date_fns_tz__WEBPACK_IMPORTED_MODULE_0_.utcToZonedTime) is not a function /events/fil-bangkok-2024 View Issue

Did you find this useful? React with a 👍 or 👎

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.

3 participants