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

Feat: workday api within about page #22

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

Feat: workday api within about page #22

wants to merge 19 commits into from

Conversation

hetd54
Copy link
Collaborator

@hetd54 hetd54 commented Nov 25, 2024

Utilizes Nextjs Route Handler in order to get job opportunities from workday.

Screenshot 2024-12-10 at 11 09 18 AM

Copy link

github-actions bot commented Nov 25, 2024

Visit the preview URL for this PR (updated for commit ecaff23):

https://ccv-website-next--pr22-feat-workday-api-e51yic13.web.app

(expires Mon, 09 Dec 2024 18:49:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 809aa2d3a6cfe026c3c7417c932015a8f1d9ec89

@hetd54 hetd54 self-assigned this Nov 26, 2024
Copy link
Collaborator

@anna-murphy anna-murphy left a comment

Choose a reason for hiding this comment

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

Looks pretty good! A few style things, and a general question.

Also, I tried to open the preview link, and I couldn't navigate to the opportunities page... Any thought as to why that was?

app/about/page.tsx Outdated Show resolved Hide resolved
app/page.tsx Outdated Show resolved Hide resolved
return (
<section
className={`"h-full p-6 flex flex-col gap-2 rounded-lg drop-shadow-md hover:drop-shadow-lg transition duration-500" ${className ? className : "bg-gray-50 border-white border-2"}`}
/* https://design-system.w3.org/components/cards.html#block-link-cards */ data-component="card"
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀
Does this work? I thought you needed some JS in there too to make this work.... BIG if true 🤔

Comment on lines +37 to +60
const handleApiCall = async () => {
try {
const res = await fetch("/api/about", {
method: "POST",
headers: {
"Content-Type": "application/json",
},
})

if (!res.ok) {
throw new Error(`Failed to fetch: ${res.status}`)
}

const opportunities = await res.json()
setData(opportunities.json)
} catch (err: any) {
setError(err.message)
} finally {
setLoading(false)
}
}
useEffect(() => {
handleApiCall()
}, [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, why not handle this in one of the server components? That way you only have to transmit the HTML of the page and you don't have to worry about loading state or whatever.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean pulling this out into its own server component? So "OpportunitiesSection" could be the html and such and Opportunities.tsx could be the server component?

I personally find it frustrating when things get too abstracted and there are 6 files that I need to look into to understand what specifically I need to edit/change; projects get overwhelming as the components folder grows; however, this is running on each page load which is probably overkill?

Thinking about it, this is worth moving for functionality; I need some help thinking about file structure in the future though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sortof! This is informed by a convention I've come to for myself. When possible, I like to do all my fetching / data interactions at the highest component level I can / is reasonable. For NextJS, it's on the page.tsx level. And I think some developers on Next JS and I are of the same opinion about this because all page.tsx components are async by default!

So the pattern would be that you do your asynchronous fetch action in the page, await the result, and then pass the resolved data to the UI component later on. For this example, it might look something like this:

app/about/page.tsx

export default async function About() {
  try {
    // Omitting function definition for brevity
    const data: OpportunityProps = await handleApiCall();
    return <Opportunities data={data} />;
  }
  catch (ex) {
    // Handle the case when the fetch fails for whatever reason, display a different page.
    return <p>{ex}</p>;
  }
}

components/Opportunities.tsx

"use client";
export function Opportunities({ data }: {data:OpportunityProps}) {
  // However you want to display the fetched data, you can omit the fetch effect because you're getting passed validated data.
  return (
    <>
      {data.map( /* ... */)}
    </>
  );
}

Does this make sense? I got a little lazy when writing this 😓

components/Opportunities.tsx Outdated Show resolved Hide resolved
Comment on lines +73 to +74
data.jobPostings.map((position) => {
return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a very small thing, but when I don't have any Logic happening inside an anonymous function like this, I tend to like to omit the return statement. Now sure if that's your vibe too, but I think the code ends up looking cleaner.

Comment on lines +87 to +91
<p className="flex items-center text-secondary-blue-700 gap-2 md:gap-4">
<MapPinIcon className="h-4 w-4" /> Providence, RI -
United States
</p>
<p className="text-gray-800">{position.title}</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wonder if things like this should be p's or span's... MDN didn't come down one way or the other, but I think there might be a reason to use something more generic here, in that this doesn't represent a paragraph in totality? Not sure though!!!

Copy link
Collaborator

@anna-murphy anna-murphy left a comment

Choose a reason for hiding this comment

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

heck, my mouse slipped at the last minute and i meant to approve.

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.

2 participants