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

Emelie Nyberg Kedert #413

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

EmelieNyberg
Copy link

@HIPPIEKICK HIPPIEKICK self-assigned this Oct 3, 2024
Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

HTML/CSS

  • Your HTML structure looks solid, but I'd consider using more semantic HTML tags instead of going for this many divs.
  • I think you managed well to follow the given design! A tiny thing would be to have a look at how the temperatures are displayed on small mobile (they don't fit on one line)

JavaScript

  • Good job structuring and naming your DOM selectors and global variables 👍
  • You haven’t implemented any error handling in case the API call fails. I recommend adding a .catch() to handle any issues, such as network errors or invalid API responses.

fetch(URL_WEATHER)
  .then(response => {
    if (!response.ok) {
      throw new Error('Weather data not found');
    }
    return response.json();
  })
  .then(data => {
    // Handle data
  })
  .catch(error => {
    console.error('Error fetching weather data:', error);
    viewCurrentWeatherText.textContent = 'Unable to retrieve weather data';
  });
  • You've already mentioned this in your Readme (great reflections by the way ✨ ), but I'm putting it here anyway: You could really benefit from creating separate functions to handle different tasks, such as updating the DOM elements, to avoid crowding the getTodaysWeather() function. This would make your code more modular and easier to maintain.

Clean Code
Your code is clean overall ⭐ When making multiple fetch requests (e.g., for the current weather and the forecast), you could refactor the fetch logic into a reusable function to reduce code repetition. Not a requirement, just an idea 😄

No changes requested, keep up the good work! 🌞

- **HTML:** Used semantic elements and classes to structure the content and facilitate dynamic updates with JavaScript.

#### Additional Thoughts
If I had more time, I would have refactored the code to break out the HTML updates from the `fetch()` function into a separate function. This would have improved the code's readability and maintainability. Additionally, I would have liked to create my own design for the app to make it more personalized, as I use weather apps like SMHI frequently. Implementing more stretch goals, such as a city selection feature or dynamically changing the page's colors based on the weather conditions, would have been interesting to explore as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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