Assignment-w2-NodeJs #2
Conversation
durw4rd
left a comment
durw4rd
left a comment
There was a problem hiding this comment.
Great job as far as programming goes! I noticed a few typos here and there and some minor opportunities for improvement, but you clearly mastered the topic taught in this module.
The main point of feedback from my side would be that you are not following the instructions very closely, and your app is actually likely rendering the wrong piece of data from the API. Lack of attention to detail can potentially be quite detrimental to the perception of your work.
| @@ -0,0 +1,5 @@ | |||
| import app from "./app.js"; | |||
| const PORT = process.env.PORT || 3000; | |||
There was a problem hiding this comment.
Great that you're using an environment variable with a fallback.
| "name": "hackyourtemperature", | ||
| "version": "1.0.0", | ||
| "description": "", | ||
| "main": "index.js", |
There was a problem hiding this comment.
This should point to server.js since that's your main file.
| "cross-env": "^10.0.0", | ||
| "jest": "^30.0.5", | ||
| "nodemon": "^3.1.10", | ||
| "superset": "^2.0.1", |
There was a problem hiding this comment.
Possibly a typo? I don't see this package used anywhere in the project.
There was a problem hiding this comment.
This is an excellent addition that was not required in the instructions 👏
| router.post("/", async (req, res) => { | ||
| const { cityName } = req.body; | ||
| if (!cityName) { | ||
| res.status(400).render("home", { errorMessage: "City is required" }); |
There was a problem hiding this comment.
There should be a return statement to prevent further execution.
| const response = await fetch(URL); | ||
|
|
||
| const data = await response.json(); | ||
| res.render("weatherShow", { data }); |
There was a problem hiding this comment.
This is a point of preference and overall API design, but the instructions asked to return temperature info for the selected city. You are returning the entire blob of data, which then needs to be parsed by the requesting client. This can work, but the client needs to be aware of this fact.
| <title>Weather show</title> | ||
| </head> | ||
| <body> | ||
| <h1>{{data.name}} <b>{{data.main.temp_min}}</b></h1> |
There was a problem hiding this comment.
I didn't test the API, but this doesn't seem like the correct field for showing the actual temperature. I think it should be data.main.temp.
There was a problem hiding this comment.
To clarify, you should have replaced this and the babe.config.csj files with the examples from the assignment instructions.
2 participants
No description provided.