-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add network query example to fetch posts and comments #87
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
base: main
Are you sure you want to change the base?
Conversation
2b321ae to
9be2fbc
Compare
lee-chase
left a comment
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.
Looks good to me, made a couple of small changes to pass linter and remove layer level which was not needed.
| try { | ||
| const response = await fetch( | ||
| // TODO: replace with actual endpoint URL | ||
| `https://jsonplaceholder.typicode.com/posts/${id}`, |
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.
I don't think we should be relying on an external provider here.
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.
@rodet To show an API call in the example without using an external provider, what pattern would you prefer? A mocked response, a local stub endpoint, or something else?
src/api/message.js
Outdated
| try { | ||
| // TODO: handle production and development environments | ||
| const response = await fetch( | ||
| `http://localhost:5173/api/comments?postId=${postId}`, |
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.
Is there any way to remove that hardcoding here?
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.
@rodet Environment variables, single source of truth config file or runtime detection?
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.
i like the idea to use a single source of truth config combined with nconf. This way the default value that stored in the config file can be overwritten by setting some env variables.
https://www.npmjs.com/package/nconf/
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.
@rodet FYI pushed change that resolves your review feedback:
Since the Express server serves both the API routes and the React app on the same origin, we can simply use relative URLs here.
i believe this converstaion can be resolved, but its your call.
rodet
left a comment
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.
Thanks for the PR @bogy0 . Let's first discuss the findings before merging this.
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…stency also removed unnecessary console.log Resolves: no-ticket Signed-off-by: [Balint Lendvai] <[email protected]>
… environments in message API functions Resolves: no-ticket Signed-off-by: [Balint Lendvai] <[email protected]>
…lanation - Updated layout to improve readability and added a detailed explanation of the data fetching process. - fixed posts responsive behaviour in mobile size - Removed unnecessary styles from the dynamic message section paragraphs. Resolves: no-ticket Signed-off-by: [Balint Lendvai] <[email protected]>
- Handled review comments - Changed error handling to set comments state correctly on failure. - Enhanced layout by introducing Stack components for better spacing and readability. - Updated paragraph elements for post body and comments for consistency. Resolves: no-ticket Signed-off-by: [Balint Lendvai] <[email protected]>
- Removed hardcoded localhost URLs for fetching posts and comments. - removed unused eslint-disabled directive import/namespace Resolves: no-ticket
…package-lock.json - Cleaned up package-lock.json by removing unnecessary 'peer' properties from various dependencies. - This was done automatically by npm after i run a fresh npm install Resolves: no-ticket Signed-off-by: [Balint Lendvai] <[email protected]>
b8de01b to
52387ff
Compare
|
FYI @rodet and @lee-chase , i had to force-push after rebasing this branch with latest |
To be able to demonstrate better external API usage introduced two new API service:
getPostgetCommentsgetMessagecreated a Posts section in the Welcome page with comments where we render these external data.
This is how that section looks like after this change:
