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

Lee Zhao Yi Charles - Weather App Bootcamp #49

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

charlesleezhaoyi
Copy link

Questions:

  1. I understand that we should not need more than 1 level of nesting for .thens, but in this specific case; wouldn't nesting create more efficiency i.e. by parsing the response of the first API call to the next directly?
`handleSubmit = (event) => {
  event.preventDefault();
  axios
    .get(
      `https://api.openweathermap.org/geo/1.0/direct?q=${this.state.cityInputValue}&limit=1&appid=${OPEN_WEATHER_API_KEY}`
    )
    // City geo data is in response.data[0]
    // Arrow functions with no curly braces return value after arrow
    .then((response) => response.data[0])
    .then((cityGeoData) =>
      axios.get(
        `https://api.openweathermap.org/data/2.5/weather?lat=${cityGeoData.lat}&lon=${cityGeoData.lon}&appid=${OPEN_WEATHER_API_KEY}&units=metric`
      )
    )
    .then((response) => {
      const { data: weatherData } = response;
      console.log(weatherData);
    });
};`

In the code block above from Rocket's documentation, I tried the code but I think we only receive the promise but not the data in the second .then block. Not sure if my understanding is accurate/correct.

@Flashrob
Copy link

Questions:

  1. I understand that we should not need more than 1 level of nesting for .thens, but in this specific case; wouldn't nesting create more efficiency i.e. by parsing the response of the first API call to the next directly?
`handleSubmit = (event) => {
  event.preventDefault();
  axios
    .get(
      `https://api.openweathermap.org/geo/1.0/direct?q=${this.state.cityInputValue}&limit=1&appid=${OPEN_WEATHER_API_KEY}`
    )
    // City geo data is in response.data[0]
    // Arrow functions with no curly braces return value after arrow
    .then((response) => response.data[0])
    .then((cityGeoData) =>
      axios.get(
        `https://api.openweathermap.org/data/2.5/weather?lat=${cityGeoData.lat}&lon=${cityGeoData.lon}&appid=${OPEN_WEATHER_API_KEY}&units=metric`
      )
    )
    .then((response) => {
      const { data: weatherData } = response;
      console.log(weatherData);
    });
};`

In the code block above from Rocket's documentation, I tried the code but I think we only receive the promise but not the data in the second .then block. Not sure if my understanding is accurate/correct.

Efficiency, depends on what you mean exactly. However, for your assumption below the code I am not sure if you are correct there. Did you try console logging? If you compare with the first api call, the second one follows the same pattern. You return the result of the axios api call, and then have access to the response. If you would use the fetch API instead of axios you might have to deal with promises however. That is my asumption here.

@@ -0,0 +1 @@
REACT_APP_API_KEY = "7b21cfd53f18535e1e40070e76eec614"

Choose a reason for hiding this comment

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

never ever commit your .env file. Always add the .env into the .gitignore file first before doing anything else. Check with git status if your .env is included in the changes before committing anything. If it is included, you set up your gitignore wrongly

@@ -21,3 +21,6 @@
npm-debug.log*
yarn-debug.log*
yarn-error.log*

# environment variables
./env

Choose a reason for hiding this comment

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

Suggested change
./env
.env

the slash here will assume a directory called env but it is a single file in the root folder


handleSubmit = (event) => {
event.preventDefault();
const APIKey = process.env.REACT_APP_API_KEY;

Choose a reason for hiding this comment

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

I would place this outside of the component, so it is always available and not has to be looked up on every form submission.

Edit <code>src/App.js</code> and save to reload.
</p>
<h2>Weather Forecast</h2>
{error !== "" ? error : ""} <br />

Choose a reason for hiding this comment

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

Suggested change
{error !== "" ? error : ""} <br />
{error && error} <br />

{error !== "" ? error : ""} <br />
{/* Display error message */}
Description: {weather_description} <br />
Temperature: {temperature === "" ? "" : temperature + " Celsius"}{" "}

Choose a reason for hiding this comment

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

Suggested change
Temperature: {temperature === "" ? "" : temperature + " Celsius"}{" "}
Temperature: {!temperature ? "" : temperature + " Celsius"}

Description: {weather_description} <br />
Temperature: {temperature === "" ? "" : temperature + " Celsius"}{" "}
<br />
Humidity: {humidity === "" ? "" : humidity + "%"}

Choose a reason for hiding this comment

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

Suggested change
Humidity: {humidity === "" ? "" : humidity + "%"}
Humidity: {!humidity ? "" : humidity + "%"}

value={city}
onChange={(e) => this.setState({ city: e.target.value })}
></input>
<button onClick={(event) => this.handleSubmit(event)}>

Choose a reason for hiding this comment

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

Suggested change
<button onClick={(event) => this.handleSubmit(event)}>
<button onClick={this.handleSubmit}>

the function in the onClick will always have the event as argument. So if you directly pass handleSubmit as reference, the event argument is still valid

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