Proper way of using redux and props

Question

I made a weather app in react and redux for an interview. The response was that I didn't use redux properly, and that it has unnecessary props. I'm not sure what they meant.

The app is on github https://github.com/s-e1/weather-app

There are 2 pages in the App,

Home contains SearchBar and HomeMain, HomeMain contains an array of ForecastCard.
Favorites contains an array of CityCard.

reducer.js

const initialState = { cityName: "Tel Aviv", cityKey: 215854, searchResults: [], weatherResults: {} };

const reducer =
    (state = initialState, action) => {
        switch (action.type) {
            case 'SET NAME':
                return { ...state, cityName: action.payload };
            case 'SET KEY':
                return { ...state, cityKey: action.payload };
            case 'SET SEARCH':
                return { ...state, searchResults: action.payload };
            case 'SET WEATHER':
                return { ...state, weatherResults: action.payload };
            default:
                return state;
        }
    }
export default reducer;

utils.js - for sending requests to the server

import axios from "axios";
const server = "https://weather-app-serv.herokuapp.com/" || "http://localhost:8000/";

export const getSearchRequest = async (cityName) => {
    if (!cityName) return [];
    let reply = await axios.get(server + "search?name=" + cityName);
    return reply.data;
}

export const getWeatherRequest = async (key) => {
    const url = `${server}weather?key=${key}`;
    let reply = await axios.get(url);
    return reply.data;
}

export const getFavoritesRequest = async (key) => {
    const url = `${server}favorites?key=${key}`;
    let reply = await axios.get(url);
    return reply.data;
}

app.js

import { useEffect } from "react";
import { Switch, Route } from "react-router-dom";
import { useDispatch, useSelector } from 'react-redux';

import Navbar from "./components/Navbar";
import Home from "./components/Home";
import Favorites from "./components/Favorites";
import { getSearchRequest, getWeatherRequest } from "./utils";
import "./App.css";

function App() {
    const dispatch = useDispatch();
    const cityName = useSelector(state => state.cityName);
    const cityKey = useSelector(state => state.cityKey);

    useEffect(() => {
        weatherCB(cityName, cityKey);
    }, [])

    const searchCB = (name) => {
        getSearchRequest(name)
            .then(data => {
                dispatch({ type: "SET SEARCH", payload: data });
            })
    }
    const weatherCB = (name, key) => {
        dispatch({ type: "SET NAME", payload: name });
        dispatch({ type: "SET KEY", payload: key });
        getWeatherRequest(key)
            .then(data => {
                dispatch({ type: "SET WEATHER", payload: data });
            })
    }
    return (
        <div className="appContainer">
            <Navbar />
            <Switch>
                <Route exact path="/"><Home searchCB={searchCB} weatherCB={weatherCB} /></Route>
                <Route path="/favorites"><Favorites weatherCB={weatherCB} /></Route>
            </Switch>
            <footer>
                <p>Data provided by <a href="https://developer.accuweather.com/">Accuweather</a></p>
            </footer>
        </div>
    );
}

export default App;

Home.js - contains SearchBar and HomeMain

import SearchBar from "./SearchBar";
import HomeMain from "./HomeMain";
import ErrorBoundary from "../ErrorBoundary";

function Home({ searchCB, weatherCB }) {
    return (
        <div>
            <ErrorBoundary>
                <SearchBar searchCB={searchCB} weatherCB={weatherCB} />
            </ErrorBoundary>

            <ErrorBoundary>
                <HomeMain />
            </ErrorBoundary>

        </div>
    );
}

export default Home;

SearchBar.js - a child of Home component

import { useState } from "react";
import { useSelector } from 'react-redux';
import "./SearchBar.css";

function SearchBar({ searchCB, weatherCB }) {
    const searchResults = useSelector(state => state.searchResults);
    const [text, setText] = useState("");

    const search = (e) => {
        setText(e.target.value);
        searchCB(e.target.value);
    }

    const sendWeather = (name, key) => {
        weatherCB(name, key);
        setText("");
        searchCB("");
    }

    return (
        <div>
            <div className="searchBar">Search: 
                <input onChange={search} value={text} />
            </div>
            {searchResults.length ?
                <ul className="searchResults">
                    {searchResults.map((e, i) => {
                        return <li className="searchItem" key={i} onClick={() => sendWeather(e.cityName, e.key)}>
                            {e.cityName}, {e.countryName}
                        </li>
                    })}
                </ul>
                : null}
        </div>
    );
}

export default SearchBar;

HomeMain.js - a child of Home component

import { useEffect, useState } from "react";
import { useSelector } from 'react-redux';

import ForecastCard from "./ForecastCard";
import "./HomeMain.css";

function HomeMain() {

    const cityName = useSelector(state => state.cityName);
    const cityKey = useSelector(state => state.cityKey);
    const weatherResults = useSelector(state => state.weatherResults);

    const [currentWeather, setCurrentWeather] = useState({});
    const [forecast, setForecast] = useState([]);
    const [imgUrl, setImgUrl] = useState("");
    const [isFavorite, setIsFavorite] = useState(false);

    const daysOfWeek = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"];
    const today = new Date().getDay();
    const iconSite = "https://developer.accuweather.com/sites/default/files/";

    useEffect(() => {
        if (weatherResults.currentWeather || weatherResults.forecast) {
            setCurrentWeather(weatherResults.currentWeather);
            setForecast(weatherResults.forecast);
            const url = iconSite + weatherResults.currentWeather.icon.padStart(2, '0') + "-s.png"
            setImgUrl(url);
            setIsFavorite(checkIfFavorite());
        }
    }, [weatherResults.currentWeather, weatherResults.forecast])

    const checkIfFavorite = () => {
        if (!localStorage.weatherApp) return false;
        const arr = JSON.parse(localStorage["weatherApp"]);
        const found = arr.some(e => e.cityName === cityName);
        return found;
    }

    const addFavorite = () => {
        setIsFavorite(true);
        let arr;
        if (!localStorage.weatherApp) {
            arr = [];
        } else {
            arr = JSON.parse(localStorage["weatherApp"]);
            const found = arr.some(e => e.cityName === cityName);
            if (found) return;
        }
        arr.push({ cityKey, cityName });
        localStorage["weatherApp"] = JSON.stringify(arr);
    }

    const removeFavorite = () => {
        setIsFavorite(false);
        const arr = JSON.parse(localStorage["weatherApp"]);
        const filteredArr = arr.filter(e => e.cityName !== cityName);
        localStorage["weatherApp"] = JSON.stringify(filteredArr);
    }

    return (
        <div className="homeMain">
            {currentWeather.text ?
                <div >
                    {isFavorite ?
                        <button onClick={removeFavorite}>Remove From Favorites</button> :
                        <button onClick={addFavorite}>Add To Favorites</button>
                    }

                    <div className="currentWeather">
                        <h4>{cityName}</h4>
                        <img src={imgUrl} alt="icon" />
                        <div>{currentWeather.temperature + '\u00B0'}c</div>
                        <div>{currentWeather.text}</div>
                    </div>
                </div>
                : null}
            {forecast.length ?
                <div className="forecast">
                    {forecast.map((e, i) => {
                        return <ForecastCard key={i} high={e.high} low={e.low} day={daysOfWeek[(today + i) % 7]}> </ForecastCard>
                    })}
                </div>
                : null}
        </div>
    );
}

export default HomeMain;

ForecastCard.js - a child of HomeMain

import "./ForecastCard.css";

function ForecastCard({ day, high, low }) {
    return (
        <div className="forecastCard">
            <strong>{day}</strong>
            <div>H: {high + '\u00B0'}c </div>
            <div>L: {low + '\u00B0'}c</div>
        </div>
    );
}

export default ForecastCard;

The 2nd page in the app for the Favorite cities weather, a child of App.

import { useEffect, useState } from "react";
import ErrorBoundary from "../ErrorBoundary";
import CityCard from "./CityCard";
import "./Favorites.css";

function Favorites({ weatherCB }) {
    const [cityArray, setCityArray] = useState([]);

    useEffect(() => {
        if (!localStorage.weatherApp) return;
        setCityArray(JSON.parse(localStorage["weatherApp"]));
    }, [])

    return (
        <div>
            <ErrorBoundary>
                <div className="favorites">
                    {cityArray.length ? cityArray.map((e, i) => {
                        return <CityCard key={i} data={e} weatherCB={weatherCB} />
                    }) : null}
                </div>
            </ErrorBoundary>
        </div>
    );
}

export default Favorites;

CityCard.js is a child of Favorites.

import { useEffect, useState } from "react";
import { useHistory } from "react-router";
import { getFavoritesRequest } from "../utils";
import "./CityCard.css";


function CityCard({ data, weatherCB }) {
    const [resData, setResData] = useState({});
    let history = useHistory();

    useEffect(() => {
        getFavoritesRequest(data.cityKey)
            .then(res => setResData(res));
    }, [data])

    const goToDetails = () => {
        history.push("/");
        weatherCB(data.cityName, data.cityKey);
    }

    return (
        <div onClick={goToDetails} className="cityCard">
            {resData.text ?
                <div>
                    <h1>{data.cityName}</h1>
                    <div>{resData.temperature + '\u00B0'}c</div>
                    <div>{resData.text}</div>
                </div>
                : null}
        </div>
    );
}

export default CityCard;

Any suggestions about improving the code are welcome.

Toby Speight · Accepted Answer · 2022-10-18 15:54:38Z

Redux has a really great style guide which outlines all of the best practices. I can use that to show which rules you are breaking to give you more specific feedback. The good news is that you are not breaking any of the rules in the "essential" category. You've got the basics down, but you need refinement on the best practices.

The response was that I didn't use redux properly, and that it has unnecessary props. I'm not sure what they meant.

Let's start with the issue of "unnecessary props". You are passing down the searchCB and weatherCB props through multiple components: from App to Home to SearchBar. You shouldn't need to do this sort of "prop drilling" with redux because the redux store is global. SearchBar can connect to redux directly, and make changes which will affect App and other components.

Connect More Components to Read Data from the Store

Let's look at the function itself, because there are a few rules to consider here:

const weatherCB = (name, key) => {
    dispatch({ type: "SET NAME", payload: name });
    dispatch({ type: "SET KEY", payload: key });
    getWeatherRequest(key)
        .then(data => {
            dispatch({ type: "SET WEATHER", payload: data });
        })
}

What we want is a single action which can be dispatched from any component anywhere in the app when the user selects a city. The action which happened is that a city was selected. You'll want to pass the city name and key in the payload.

There's a rule specifically for this next one, but...

const [currentWeather, setCurrentWeather] = useState({});
const [forecast, setForecast] = useState([]);
const [imgUrl, setImgUrl] = useState("");

/*... */

useEffect(() => {
    if (weatherResults.currentWeather || weatherResults.forecast) {
        setCurrentWeather(weatherResults.currentWeather);
        setForecast(weatherResults.forecast);
        const url = iconSite + weatherResults.currentWeather.icon.padStart(2, '0') + "-s.png"
        setImgUrl(url);
        setIsFavorite(checkIfFavorite());
    }
}, [weatherResults.currentWeather, weatherResults.forecast])

In your HomeMain component you are copying state that already exists in redux into the local state of your component. You don't want to do this. Just use the redux state directly, and make use of selector functions if you want to derive values from it.

I'm rewriting your reducer.js file using thunks, and that will allow us to do some cleanup in the components.

import { createSlice, createAsyncThunk } from "@reduxjs/toolkit";
import { getSearchRequest, getWeatherRequest } from "./utils";

const initialState = {
  city: { name: "Tel Aviv", key: 215854 },
  searchResults: [],
  weatherResults: {}
};

export const fetchSearchResults = createAsyncThunk(
  "weather/fetchSearchResults",
  getSearchRequest
);

export const fetchWeather = createAsyncThunk(
  "weather/fetchWeather",
  getWeatherRequest
);

const weatherSlice = createSlice({
  name: "weather",
  initialState,
  reducers: {
    selectCity: (state, action) => {
      state.city = action.payload;
    }
  },
  extraReducers: (builder) =>
    builder
      .addCase(fetchSearchResults.fulfilled, (state, action) => {
        state.searchResults = action.payload;
      })
      .addCase(fetchWeather.fulfilled, (state, action) => {
        state.weatherResults = action.payload;
      })
});

export default weatherSlice.reducer;

export const { selectCity } = weatherSlice.actions;

This requires a little change in index.js to enable thunk middleware and other redux-toolkit features.

import { configureStore } from "@reduxjs/toolkit";

const store = configureStore({ reducer });

Home and SearchBar should not take any props.

SearchBar involves two actions: selecting a city and submitting a search request.

const dispatch = useDispatch();

const search = (e) => {
  setText(e.target.value);
  dispatch(fetchSearchResults(e.target.value));
};

const sendWeather = (name, key) => {
  dispatch(selectCity({ name, key }));
  setText("");
};

App does not need any redux logic at all! All of the subcomponents can handle that themselves. So you can delete a lot here.

HomeMain can be responsible for initiating the weather fetching for the currently selected city. We also want to get rid of all of the redundant state.

const cityName = useSelector((state) => state.cityName);
const cityKey = useSelector((state) => state.cityKey);
const weatherResults = useSelector((state) => state.weatherResults);

const { currentWeather, forecast } = weatherResults;

const dispatch = useDispatch();

useEffect(() => {
  dispatch(fetchWeather(cityKey));
}, [dispatch, cityKey]);

Complete Revised Code

Stack Exchange Network

Proper way of using redux and props

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Proper way of using redux and props

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions