Cr review

.gitignore

@@ -18,3 +18,4 @@
 npm-debug.log*
 yarn-debug.log*
 yarn-error.log*
+.env

notes.txt

@@ -0,0 +1,4 @@
+public assets divition into folders in redundant
+package.json has no division to devDeps
+
+mix of sass and mui styles, confusing

src/components/AlertMessage.component.tsx

@@ -1,11 +1,13 @@
-import Alert from '@mui/material/Alert';
-import { AlertColor } from '@mui/material/Alert/Alert';
+import SeverityType from './types/Severity.type';
+
+import Alert, { AlertColor } from '@mui/material/Alert';
 
 interface Props {
   message: string;
   severity: AlertColor;
 }
 
+// this comp has no logic nor styling, just use mui Alert.
 const AlertMessage: React.FC<Props> = ({ message, severity }) => {
   return (
     <Alert severity={severity}>{message}</Alert>

src/components/BreweriesList.component.tsx

@@ -15,6 +15,7 @@ interface Props {
 }
 
 const BreweriesList: React.FC<Props> = ({ breweries, page }) => {
+  /* if we are already using state and are aware of page, why prop for breweries? */
   const { isLoading, isError } = useAppSelector((state) => state.brewery);
 
   return (

src/components/Brewery.component.tsx

@@ -21,6 +21,7 @@ interface Props {
 const Brewery: React.FC<Props> = ({ brewery }) => {
   const dispatch = useAppDispatch();
 
+  /* state should probably add 'isFavorite' prop for each brewery */
   const { favoredBreweries } = useAppSelector((state) => state.brewery);
 
   const isFavored = favoredBreweries.find((item) => item.id === brewery.id);
@@ -52,7 +53,7 @@ const Brewery: React.FC<Props> = ({ brewery }) => {
         <Button onClick={handleClickToggle}
           size='large'
           sx={{ fontWeight: 800 }}>
-          {isFavored ? <FavoriteIcon color='error' /> : <FavoriteBorderIcon color='error' />}
+          {isFavored ? <FavoriteIcon color='error' /> : <FavoriteBorderIcon color='error' />}{/* color error is wierd */}
         </Button>
       </CardActions>
     </Card>

src/components/Header.component.tsx

@@ -1,5 +1,5 @@
-import * as React from 'react';
-import { useNavigate } from 'react-router-dom';
+import * as React from "react";
+import { useNavigate } from "react-router-dom";
 import { Link } from "react-router-dom";
 
 import {
@@ -11,17 +11,19 @@ import {
   Typography,
   IconButton,
   Toolbar,
-} from '@mui/material';
-import MenuIcon from '@mui/icons-material/Menu';
+} from "@mui/material";
+import MenuIcon from "@mui/icons-material/Menu";
 
-import { LogoTypography, HeaderBox } from './';
+import { LogoTypography, HeaderBox } from "./";
 
-import { pages } from '../constants';
+import { pages } from "../constants";
 
-import './styles/Header.style.scss';
+import "./styles/Header.style.scss";
 
 const Header = () => {
-  const [anchorElNav, setAnchorElNav] = React.useState<null | HTMLElement>(null);
+  const [anchorElNav, setAnchorElNav] = React.useState<null | HTMLElement>(
+    null
+  );
   const navigate = useNavigate();
 
   const handleOpenNavMenu = (event: React.MouseEvent<HTMLElement>) => {
@@ -41,12 +43,11 @@ const Header = () => {
     <AppBar position="static" color="primary">
       <Container maxWidth="xl">
         <Toolbar disableGutters>
-          <Link to='/' className='link'>
-            <LogoTypography isMobile={false} title='My Breweries' />
+          <Link to="/" className="link">
+            <LogoTypography isMobile={false} title="My Breweries" />
           </Link>
-          <img src="assets/images/logo/logo.png" alt="logo" className='logo' />
-
-          <HeaderBox isMobile={true} >
+          <img src="assets/images/logo/logo.png" alt="logo" className="logo" />
+          <HeaderBox isMobile={true}>
             <IconButton
               size="large"
               aria-label="account of current user"
@@ -61,37 +62,40 @@ const Header = () => {
               id="menu-appbar"
               anchorEl={anchorElNav}
               anchorOrigin={{
-                vertical: 'bottom',
-                horizontal: 'left',
+                vertical: "bottom",
+                horizontal: "left",
               }}
               keepMounted
               transformOrigin={{
-                vertical: 'top',
-                horizontal: 'left',
+                vertical: "top",
+                horizontal: "left",
               }}
               open={Boolean(anchorElNav)}
               onClose={handleCloseNavMenu}
               sx={{
-                display: { xs: 'block', md: 'none' },
+                display: { xs: "block", md: "none" },
               }}
             >
               {pages.map((page) => (
-                <MenuItem key={page.name} onClick={() => handleOnClickNavMenu(page.path)}>
+                <MenuItem
+                  key={page.name}
+                  onClick={() => handleOnClickNavMenu(page.path)}
+                >
                   <Typography textAlign="center">{page.name}</Typography>
                 </MenuItem>
               ))}
             </Menu>
           </HeaderBox>
-          <Link to='/' className='link'>
-            <LogoTypography isMobile={true} title='My Breweries' />
-          </Link>
 
+          <Link to="/" className="link">
+            <LogoTypography isMobile={true} title="My Breweries" />
+          </Link>
           <HeaderBox isMobile={false}>
             {pages.map((page) => (
               <Button
                 key={page.name}
                 onClick={() => handleOnClickNavMenu(page.path)}
-                sx={{ my: 2, mr: 5, color: 'white', display: 'block' }}
+                sx={{ my: 2, mr: 5, color: "white", display: "block" }}
               >
                 {page.name}
               </Button>
@@ -103,4 +107,4 @@ const Header = () => {
   );
 };
 
-export default Header;
+export default Header;

src/components/types/Severity.type.ts

@@ -0,0 +1,4 @@
+// this is a type from mui (AlertColor), why write it again?
+type SeverityType = 'success' | 'error' | 'warning' | 'info';
+
+export default SeverityType;

src/constants/index.ts

@@ -1,3 +1,4 @@
+/* pages are defined both here and in App.tsx, also naming inconsistency */
 export const pages = [
   { name: 'Home', path: '/' },
   { name: 'Favorite Breweries', path: '/favorites' },

src/features/breweries/brewerySlice.ts

@@ -1,11 +1,12 @@
-import { AxiosError } from 'axios';
-import { createAsyncThunk, createSlice, PayloadAction } from '@reduxjs/toolkit';
+import { AxiosError } from "axios";
+import { createAsyncThunk, createSlice, PayloadAction } from "@reduxjs/toolkit";
 
-import { BreweryState, IBrewery } from './interfaces/Brewery.interfaces';
-import { breweryService, errorsService } from '../../services';
+import { BreweryState, IBrewery } from "./interfaces/Brewery.interfaces";
+import { breweryService, errorsService } from "../../services";
 
-import { LOCAL_STORAGE_KEY } from '../../constants';
+import { LOCAL_STORAGE_KEY } from "../../constants";
 
+// initial state should probably read fron local storage
 const initialState: BreweryState = {
   breweries: [],
   favoredBreweries: [],
@@ -14,19 +15,23 @@ const initialState: BreweryState = {
   isError: false,
 };
 
-export const getBreweries = createAsyncThunk('breweries', async () => {
+export const getBreweries = createAsyncThunk("breweries", async () => {
   try {
     const breweries = await breweryService.getBreweries();
     return breweries;
   } catch (err) {
+    // this is called also in breweryService.getBreweries
     const error = err as AxiosError;
     errorsService.handleError(error);
   }
 });
 
 export const getFavoredBreweriesFromAPI = createAsyncThunk(
-  'favoredBreweries',
-  async (favoriteBreweriesIds: string[]) => {
+  "favoredBreweries",
+  async (
+    favoriteBreweriesIds: string[],
+    { /* this is used */ rejectWithValue }
+  ) => {
     try {
       const favoredBreweries = await breweryService.getFavoredBreweriesFromAPI(
         favoriteBreweriesIds
@@ -43,13 +48,14 @@ const toggleFavoriteBrewery = (
   favoredBreweries: IBrewery[],
   selectedBrewery: IBrewery
 ) => {
+  // no need for all the spreads...
   let newFavoredBreweries = [...favoredBreweries];
   const favoredBrewery = newFavoredBreweries.find(
     (brewery) => brewery.id === selectedBrewery.id
   );
 
   if (!favoredBrewery) {
-    newFavoredBreweries.push(selectedBrewery);
+    newFavoredBreweries.push({ ...selectedBrewery });
   } else {
     newFavoredBreweries = newFavoredBreweries.filter(
       (brewery) => brewery.id !== favoredBrewery.id
@@ -60,16 +66,20 @@ const toggleFavoriteBrewery = (
     (brewery: IBrewery) => brewery.id
   );
 
+  // multiple places using local storage, not a good idea.
   localStorage.setItem(LOCAL_STORAGE_KEY, JSON.stringify(favoredBreweriesIds));
 
   return newFavoredBreweries;
 };
 
 export const brewerySlice = createSlice({
-  name: 'brewery',
+  name: "brewery",
   initialState,
   reducers: {
-    toggleFavorite: (state, action: PayloadAction<IBrewery>) => {
+    toggleFavorite: (
+      state,
+      /* why not just passing id? */ action: PayloadAction<IBrewery>
+    ) => {
       const modifiedFavoredBreweries = toggleFavoriteBrewery(
         state.favoredBreweries,
         action.payload

src/features/breweries/interfaces/Brewery.interfaces.ts

@@ -1,3 +1,4 @@
+// this isnt C#, no ISomething
 export interface IBrewery {
   id: string;
   name: string;
@@ -9,14 +10,18 @@ export interface IBrewery {
   website_url: string | null;
 }
 
+
 export interface AsyncState {
   isLoading: boolean;
   isSuccess: boolean;
   isError: boolean;
 }
 
+
+// listed in the wrong order
 export interface BreweryState extends AsyncState {
   breweries: IBrewery[];
+  // why are favored stored whole instead of just ids?
   favoredBreweries: IBrewery[];
 }
 

src/hooks/useLocalStorage.ts

@@ -12,14 +12,14 @@ const useLocalStorage = () => {
   const storedFavoredBreweriesIds: string | null =
     localStorage.getItem(LOCAL_STORAGE_KEY);
 
-  const favoredBreweriesIds: string[] | [] = useMemo(() => {
+  const favoredBreweriesIds: string[] | null = useMemo(() => {
     return storedFavoredBreweriesIds
       ? JSON.parse(storedFavoredBreweriesIds)
       : [];
   }, [storedFavoredBreweriesIds]);
 
   useEffect(() => {
-    if (favoredBreweriesIds.length > 0) {
+    if (Array.isArray(favoredBreweriesIds)) {
       dispatch(getFavoredBreweriesFromAPI(favoredBreweriesIds));
     }
   }, [dispatch, favoredBreweriesIds]);

src/services/brewery.service.ts

@@ -1,7 +1,7 @@
-import axios, { AxiosError } from 'axios';
+import axios, { AxiosError } from "axios";
 
-import { IBrewery } from '../features/breweries/interfaces/Brewery.interfaces';
-import { errorsService } from './';
+import { IBrewery } from "../features/breweries/interfaces/Brewery.interfaces";
+import { errorsService } from "./";
 
 const getBreweries = async () => {
   try {
@@ -19,9 +19,11 @@ const getFavoredBreweriesFromAPI = async (favoredBreweries: string[]) => {
   try {
     const promises: Promise<{ data: IBrewery }>[] = [];
 
+    // should map favoredBreweries into promises instead.
+    // also should add type to axios.get instead of promises.
     favoredBreweries.forEach((id: string) => {
       promises.push(
-        axios.get(`${process.env.REACT_APP_BASE_URL}/breweries/${id}`)
+        axios.get<IBrewery>(`${process.env.REACT_APP_BASE_URL}/breweries/${id}`)
       );
     });
 

src/services/errors.service.ts

@@ -1,7 +1,9 @@
 import { AxiosError } from 'axios';
 
+// this should have beed used in axios.interceptors...
 const handleError = (error: AxiosError) => {
   console.error('Error: ', error);
+  // why throw new?
   throw new Error(error.message);
 };
 

src/store.ts

@@ -6,6 +6,10 @@ export const store = configureStore({
   reducer: {
     brewery: breweryReducer,
   },
+  middleware: (getDefaultMiddleware) =>
+    getDefaultMiddleware({
+      serializableCheck: false,
+    }),
 });
 
 export type RootState = ReturnType<typeof store.getState>;

src/utils/theme.ts

@@ -1,5 +1,6 @@
 import { createTheme } from '@mui/material/styles';
 
+// why under utils folder? 
 export const theme = createTheme({
   palette: {
     primary: {