Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feat: workday api within about page #22
Changes from 17 commits
72ca4e5
e1830e0
98b1a18
4febae6
4e89709
26d79fa
763d046
075b03b
c860637
cd58219
830b63d
f896d45
7ab4f77
07c2943
ecaff23
296d237
f318f6e
e93df46
29304cc
db8858e
e0c9ae5
a1a3045
ae13bb3
508f86b
4b6ddc8
4a2c824
a618c91
065cd57
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
👀
Does this work? I thought you needed some JS in there too to make this work.... BIG if true 🤔
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.
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.
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 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!
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.
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 allpage.tsx
components areasync
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
components/Opportunities.tsx
Does this make sense? I got a little lazy when writing this 😓
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.
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.
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.
Wonder if things like this should be
p
's orspan
'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!!!