Add /events/[year]/[term] page #158

Merged
a3thakra merged 46 commits from feat/events-term into main 1 year ago
j285he commented 1 year ago
Collaborator

Closes #113

Closes #113
j285he added 36 commits 1 year ago
b733acd156 Add lib file
bfcbf5c687 Fix type error
9f7d40b426 Merge branch 'main' into feat/events-lib
c39b0036ad Correct Dirents
590b2554c0 Fixes for dirents
d237621d14 ESLint fixes
3a697c09c0 Add sorting
b32fd53d95 Remove unneeded dirent conversion
40d37c4aea Fix lint problems
fecd3bf3e0 Rename functions
2798efc221 Function renaming
5dd258ec86 Merge branch 'main' into feat/events-year
408275e8cf Remove temp page
10449e921e Remove events
6f429d20bf Add events
29f2c3208c Fix build error
50c2f08769 Simplify types
a23a10d8e4 Fix for Fall 2018
416e027ebb Add futureEvents pastEvents EventCard logic
e971c85afc Create events/index.tsx
5631217681 Add time detection for current time
3e61407643 Remove events
j285he requested review from a3thakra 1 year ago
a3thakra reviewed 1 year ago
import React from "react";
import { EventCard } from "../../../../components/EventCard";
import { MiniEventCard } from "../../../../components/MiniEventCard";
Collaborator

Use "@/components/..." instead of "../../../../components/..."

Use "@/components/..." instead of "../../../../components/..."
j285he marked this conversation as resolved
a3thakra reviewed 1 year ago
getEventsByTerm,
getEventBySlug,
Event,
} from "../../../../lib/events";
Collaborator

use "@/lib/events" instead of "../../../lib/events"

use "@/lib/events" instead of "../../../lib/events"
j285he marked this conversation as resolved
a3thakra reviewed 1 year ago
import styles from "./term.module.css";
export async function getStaticPaths(): Promise<{
Collaborator

This is not the correct way to type out getStaticPaths. You should do something like the news archive files.

export const getStaticPaths: GetStaticPaths = async () => {...} something like this

This is not the correct way to type out `getStaticPaths`. You should do something like the news archive files. `export const getStaticPaths: GetStaticPaths = async () => {...}` something like this
j285he marked this conversation as resolved
a3thakra reviewed 1 year ago
const years = await getEventYears();
const paths = [];
for (const year of years) {
const terms = await getEventTermsByYear(year);
Collaborator

This is inefficient. It always wait for 1 year to complete before proceeding to the next. You should use Promise.all here.

This is inefficient. It always wait for 1 year to complete before proceeding to the next. You should use Promise.all here.
Poster
Collaborator

I'm a little confused here. The problem is that await getEventTermsByYear(year) forces the function to stop until getEventTermsByYear returns, right?

But on the next line I am getting an error with being unable to map on a type of Promise<string[]>. How can I resolve this?

Thanks!

I'm a little confused here. The problem is that `await getEventTermsByYear(year)` forces the function to stop until `getEventTermsByYear` returns, right? But on the next line I am getting an error with being unable to map on a type of `Promise<string[]>`. How can I resolve this? Thanks!
Collaborator

yes.

you can await on a Promise.all to map over it.

so something like:

const paths = (await Promise.all(
  years.map(async (year) => {
    const terms = await getEventTermsByYear(year);
    return terms.map((curTerm) => ({ params: { year: year, term: curTerm } }))
  })
)).flat()
yes. you can `await` on a Promise.all to map over it. so something like: ```ts const paths = (await Promise.all( years.map(async (year) => { const terms = await getEventTermsByYear(year); return terms.map((curTerm) => ({ params: { year: year, term: curTerm } })) }) )).flat() ```
Poster
Collaborator

Would the use of await getEventTermsByYear here still force the async function on line 2 to wait until getEventTermsByYear returns?

Is the difference here that years.map can map each element asynchronously, whereas in my original function for (const year of years) cannot?

Would the use of `await getEventTermsByYear` here still force the async function on line 2 to wait until `getEventTermsByYear` returns? Is the difference here that `years.map` can map each element asynchronously, whereas in my original function `for (const year of years)` cannot?
Collaborator

the for loop is going to wait for each year to be completed before the next can start.

But with this approach, all years start simultaneously.

Within each year, we are still waiting for terms, that doesn't change in both cases.

2020
  - winter
  - spring
  - fall
2021
  - winter
  - spring
  - fall

vs

2020               | 2021
  - winter         |   - winter
  - spring         |   - spring
  - fall           |   - fall
the for loop is going to wait for each year to be completed before the next can start. But with this approach, all years start simultaneously. Within each year, we are still waiting for terms, that doesn't change in both cases. ``` 2020 - winter - spring - fall 2021 - winter - spring - fall ``` vs ``` 2020 | 2021 - winter | - winter - spring | - spring - fall | - fall ```
Poster
Collaborator

Ah that makes sense, thank you!

Ah that makes sense, thank you!
j285he marked this conversation as resolved
a3thakra reviewed 1 year ago
const events = (
await Promise.all(
(
await getEventsByTerm(context.params.year, context.params.term)
Collaborator

This is a lot of nesting, can you move this to a separate line, and bind that to a variable?

This is a lot of nesting, can you move this to a separate line, and bind that to a variable?
j285he marked this conversation as resolved
a3thakra reviewed 1 year ago
margin: auto;
margin-top: 7rem;
margin-bottom: 14rem;
width: 50rem;
Collaborator

Can you convert these rem values to pixels? Using the calc(<value in px>rem / 16) method that we use everywhere.

Can you convert these rem values to pixels? Using the `calc(<value in px>rem / 16)` method that we use everywhere.
j285he marked this conversation as resolved
a3thakra reviewed 1 year ago
}
.miniEventCards {
margin-top: 30px;
Collaborator

This should be in rems

This should be in rems
j285he marked this conversation as resolved
a3thakra reviewed 1 year ago
};
}
const Term = (props: Props) => {
Collaborator

In all the other files, we place the react component at the top of the file, we should do that here too, for consistency. Also you can use export default function Term(props: Props) {...} syntax here, instead of the anonymous function.

In all the other files, we place the react component at the top of the file, we should do that here too, for consistency. Also you can use `export default function Term(props: Props) {...}` syntax here, instead of the anonymous function.
j285he marked this conversation as resolved
j285he added 2 commits 1 year ago
j285he added 2 commits 1 year ago
3a6f799538 Optimize getStaticPaths
j285he added 2 commits 1 year ago
j285he added 1 commit 1 year ago
24885e5185 Add margin-bottom for terms
a3thakra added 1 commit 1 year ago
2abc7e75c1 PR fixes
j285he added 1 commit 1 year ago
8d59ae1692 Add getNextTerm, getPreviousTerm
a3thakra added 1 commit 1 year ago
24a237a8c6 Add header functionality (#183)
a3thakra approved these changes 1 year ago
a3thakra merged commit e548bd9c5a into main 1 year ago
a3thakra deleted branch feat/events-term 1 year ago
j285he referenced this issue from a commit 1 year ago

Reviewers

a3thakra approved these changes 1 year ago
continuous-integration/drone/push Build is passing
The pull request has been merged as e548bd9c5a.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#158
Loading…
There is no content yet.