Project Weather App #429
Open
Project Weather App #429
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ors and API URL and endpoints
…d that came along with the instructions, to avoid security leak mail from GitHub
…mperatur. Update DOM selectors and HTML with correct ID and names.
…ngitud, and add forecast BASE_URL and forecast URL.
…a global const and place the const for todayURL inside the function, and finally add city as a parameter to that function. Test dynamic city by adding Las Vegas as a param in function invocation
…and Temp. JS: Fix forecast filtering, add forEach loop to display forecast at 12:00 and to add day name using weekdays array.
… 1000, which the date object requires
…ns for sun, cloudy and few clouds.
…ay for comparison in the forecast.
JennieDalgren
previously requested changes
Oct 4, 2024
There was a problem hiding this comment.
Good job with the project.
Great work with
- Async/Await Usage: The code uses async/await for fetching data, which makes it more readable and easier to handle.
- Error Handling: There are try/catch blocks to handle errors during API calls.
- DOM Manipulation: The code dynamically updates the DOM with weather data
- Separation of functions: Functions are well-defined and divided up into understandable chunks.
Things to fix:
It feels strange with a sunrise at 15:38.
Either show the local time or change the design to clearly show the user that the sunrise and sunset times are shown in the timezone the user is in. Or both.
|
Hi Jennie,
thanks for pointing that out! The app now shows sunrise and sunset in local
time.
Kindest regards / Med vänlig hälsning,
*====================*
*Johanna Eriksson*
Schibsted Enterprise Tech
Västra Järnvägsgatan 21
105 17 Stockholm
***@***.*** ***@***.***>
…On Fri, 4 Oct 2024 at 15:48, Jennie Dalgren ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Good job with the project.
*Great work with*
1. *Async/Await Usage:* The code uses async/await for fetching data,
which makes it more readable and easier to handle.
2. *Error Handling:* There are try/catch blocks to handle errors
during API calls.
3. *DOM Manipulation:* The code dynamically updates the DOM with
weather data
4. *Separation of functions:* Functions are well-defined and divided
up into understandable chunks.
*Things to fix:*
image.png (view on web)
<https://github.com/user-attachments/assets/f9c0fb7d-2118-43dc-8a71-12a3c7a170f9>
It feels strange with a sunrise at 15:38.
Either show the local time or change the design to clearly show the user
that the sunrise and sunset times are shown in the timezone the user is in.
Or both.
—
Reply to this email directly, view it on GitHub
<#429 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKKOL7OVRM7K3GCEQIHCRE3ZZ2MCTAVCNFSM6AAAAABPCCQWHKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNBYGEYTKNRRGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
HIPPIEKICK
approved these changes
Dec 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Netlify link
https://project-weather-app-by-joe.netlify.app