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.

Sign up for GitHub

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

Jump to bottom

code review Raul & Ottavia #1

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

code review Raul & Ottavia #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9d3304
code review Raul & Ottavia
OttaviaFeletig Jun 30, 2022
84c12f6
protected route fix when refreshing page
OttaviaFeletig Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 34 additions & 20 deletions src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,54 @@
import './App.css';
import 'bootstrap/dist/css/bootstrap.min.css';
import { Routes, Route} from 'react-router-dom';
import Home from './views/Home.js';
import Detail from './views/Detail.js';
import Register from './views/Register.js';
import About from './views/About.js';
import ErrorPage from './views/ErrorPage.js';
import Nav from './components/Nav.js';
import { AuthContextProvider } from './context/AuthContext';
import ProtectedRoute from './components/ProtectedRoute';
import { app } from "./config"
import { Container } from 'react-bootstrap';
import Login from './views/Login';
import MyRack from './views/MyRack';
import "./App.css";
import "bootstrap/dist/css/bootstrap.min.css";
import { Routes, Route } from "react-router-dom";
import Home from "./views/Home.js";
import Detail from "./views/Detail.js";
import Register from "./views/Register.js";
import About from "./views/About.js";
import ErrorPage from "./views/ErrorPage.js";
import Nav from "./components/Nav.js";
import { AuthContextProvider } from "./context/AuthContext";
import ProtectedRoute from "./components/ProtectedRoute";
import { app } from "./config";
import { Container } from "react-bootstrap";
import Login from "./views/Login";
import MyRack from "./views/MyRack";

function App() {
const year = new Date().getFullYear().toString();

return (
<Container className='App'>
<Container className="App">
<AuthContextProvider>
<Nav />
<Routes>
<Route path="/" element={<Home />} />
<Route path="detail/:bookid" element={<ProtectedRoute><Detail /></ProtectedRoute>} />
<Route
path="detail/:bookid"
element={
<ProtectedRoute>
<Detail />
</ProtectedRoute>
}
/>
<Route path="register" element={<Register />} />
<Route path="login" element={<Login />} />
<Route path="myrack" element={<ProtectedRoute><MyRack /></ProtectedRoute>} />
<Route
path="myrack"
element={
<ProtectedRoute>
<MyRack />
</ProtectedRoute>
}
/>
<Route path="about" element={<About />} />
<Route path="*" element={<ErrorPage />} />
</Routes>
</AuthContextProvider>

<footer className='pt-3'>© {year} Marta Podziewska</footer>
<footer className="pt-3">© {year} Marta Podziewska</footer>
</Container>
)
);
}

export default App;
128 changes: 71 additions & 57 deletions src/components/BookList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,68 +1,82 @@
import React, { useState, useEffect } from 'react'
import Pagination from './Pagination';
import { Col, Container, Row } from 'react-bootstrap';
import BookPreview from './BookPreview';

import React, { useState, useEffect } from "react";
import Pagination from "./Pagination";
import { Col, Container, Row } from "react-bootstrap";
import BookPreview from "./BookPreview";
// ! general observation: everytime you fetch the API with a different startIndex you get back a different totalItems number
// ! so, when you are on page 1 and you click on page 70, your app crashes. did you notice it? do you have an idea why the API gives you different numbers of books?
function BookList() {
const [books, setBooks] = useState([]);
const [currentIndex, setCurrentIndex] = useState(0);
const [totalBookCount, setTotalBookCount] = useState(0)
const [totalPageCount, setTotalPageCount] = useState(0)
const [url, setUrl] = useState("https://www.googleapis.com/books/v1/volumes?q=climbing&key="+process.env.REACT_APP_GOOGLE_BOOKS_API_KEY+"&startIndex=");
const [books, setBooks] = useState([]);
const [currentIndex, setCurrentIndex] = useState(0);
const [totalBookCount, setTotalBookCount] = useState(0);
const [totalPageCount, setTotalPageCount] = useState(0);
const [url, setUrl] = useState(
"https://www.googleapis.com/books/v1/volumes?q=climbing&key=" +
process.env.REACT_APP_GOOGLE_BOOKS_API_KEY +
"&startIndex="
);

const fetchBooks = async () => {
try {
const response = await fetch(url+currentIndex);
const bookData = await response.json();
const bookCount = await bookData.totalItems;
setTotalBookCount(bookCount);
const numberOfPages = Math.ceil(bookCount/10)
setTotalPageCount(numberOfPages);
const bookList = await bookData.items;
setBooks(bookList);
} catch (err) {
console.log(err);
};
};
const fetchBooks = async () => {
try {
const response = await fetch(url + currentIndex);
const bookData = await response.json();
console.log("bookData", bookData);
// ! why do you have to await for the total items? totalItems is just a key of the bookData object
const bookCount = await bookData.totalItems;
setTotalBookCount(bookCount);
const numberOfPages = Math.ceil(bookCount / 10);
setTotalPageCount(numberOfPages);
// ! same here, items is part of the object so you don't have to wait
const bookList = await bookData.items;
setBooks(bookList);
} catch (err) {
// ! here it would be good to set an error message and display it to the user if it occurs
console.log(err);
}
};

const handlePageClick = (event) => {
setCurrentIndex(event.selected)
};
const handlePageClick = (event) => {
setCurrentIndex(event.selected);
};

useEffect(() => {
fetchBooks()
}, [currentIndex])
useEffect(() => {
fetchBooks();
}, [currentIndex]);

return (
<>
<Row>
<Col>
{totalBookCount ? <p>A total of {totalBookCount} results, page {currentIndex+1}</p> :
<div className='py-3'>
<p>Loading list view...</p>
</div>
}
</Col>
</Row>
<Container className='d-flex flex-wrap'>
{books && books.map(book => {
return (
<Container
className='py-1'
key={book.id}
>
<BookPreview props={book}/>
</Container>
)
}
)}
</Container>
<Row>
<Col>
{totalBookCount ? (
<p>
A total of {totalBookCount} results, page {currentIndex + 1}
</p>
) : (
<div className="py-3">
<p>Loading list view...</p>
</div>
)}
</Col>
</Row>
<Container className="d-flex flex-wrap">
{books &&
books.map((book) => {
return (
<Container className="py-1" key={book.id}>
{/*//! calling props a prop is a bit weird, why don't you call it book? */}
<BookPreview props={book} />
</Container>
);
})}
</Container>

{totalPageCount &&
<Pagination handlePageClick={handlePageClick} totalPageCount={totalPageCount}
/>}
{totalPageCount && (
<Pagination
handlePageClick={handlePageClick}
totalPageCount={totalPageCount}
/>
)}
</>
)
);
}

export default BookList
export default BookList;
59 changes: 31 additions & 28 deletions src/components/BookPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,38 @@
import React from 'react'
import { Col, Image, Row } from 'react-bootstrap'
import FavButton from './FavButton'
import SeeDetailButton from './SeeDetailButton'

function BookPreview({props}) {
import React from "react";
import { Col, Image, Row } from "react-bootstrap";
import FavButton from "./FavButton";
import SeeDetailButton from "./SeeDetailButton";

function BookPreview({ props }) {
return (
<>
<Row
className='justify-content-center'
>
{/* <Col xs={2} md={1} className='d-flex'>
<>
<Row className="justify-content-center">
{/* <Col xs={2} md={1} className='d-flex'>
<SeeDetailButton id={props.id} />
<FavButton/>
</Col> */}
<Col>
<h5>{props.volumeInfo.title} </h5>
</Col>
</Row>
<Row>
<Col xs={4} md={3} l={2}>
<Image thumbnail="true" fluid="true" src={props.volumeInfo.imageLinks.thumbnail} alt={`Cover photo of the book ${props.volumeInfo.title}`}/>
</Col>
<Col>
<p>{`${props.volumeInfo.description.slice(0, 100)}...`}</p>
<SeeDetailButton id={props.id} />
<FavButton/>
</Col>
</Row>
</>
)
<Col>
<h5>{props.volumeInfo.title} </h5>
</Col>
</Row>
<Row>
<Col xs={4} md={3} l={2}>
<Image
thumbnail="true"
fluid="true"
src={props.volumeInfo.imageLinks.thumbnail}
alt={`Cover photo of the book ${props.volumeInfo.title}`}
/>
</Col>
<Col>
<p>{`${props.volumeInfo.description.slice(0, 100)}...`}</p>
{/* // ! nice that you created specific components for it */}
<SeeDetailButton id={props.id} />
<FavButton />
</Col>
</Row>
</>
);
}

export default BookPreview
export default BookPreview;
92 changes: 51 additions & 41 deletions src/components/BookReviews.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,54 +1,64 @@
import { collection, getDocs, query, where } from 'firebase/firestore';
import React, { useContext, useEffect, useState } from 'react'
import { Container } from 'react-bootstrap';
import { useParams } from 'react-router-dom';
import { db } from '../config'
import { AuthContext } from '../context/AuthContext';
import NoteCard from './NoteCard';
import { collection, getDocs, query, where } from "firebase/firestore";
import React, { useContext, useEffect, useState } from "react";
import { Container } from "react-bootstrap";
import { useParams } from "react-router-dom";
import { db } from "../config";
import { AuthContext } from "../context/AuthContext";
import NoteCard from "./NoteCard";

function BookReviews() {
const [reviews, setReviews] = useState()
let { bookid } = useParams();
const {user} = useContext(AuthContext);
const bookReviews = query(collection(db, "notes"), where("book_id", "==", bookid), where("user", "!=", user.email));
const [reviews, setReviews] = useState();
let { bookid } = useParams();
const { user } = useContext(AuthContext);
// ! why are you hiding the logged in user's notes? please remember to tell us cause we are very curious

const getReviews = async() => {
try {
const querySnapshot = await getDocs(bookReviews);
let fetchedReviews = [];
querySnapshot.forEach((doc) => {
fetchedReviews.push(doc.data());
});
setReviews(fetchedReviews)
const getReviews = async () => {
// ! the bookReviews query should be here because if you don't have the user yet (we know it's a protected route, but still we are trying to break your code as much as we can ;))
// ! it will throw an error
const bookReviews = query(
collection(db, "notes"),
where("book_id", "==", bookid),
where("user", "!=", user.email)
);

} catch (error) {
console.log(error)
}
};
try {
const querySnapshot = await getDocs(bookReviews);
let fetchedReviews = [];
querySnapshot.forEach((doc) => {
fetchedReviews.push(doc.data());
});
setReviews(fetchedReviews);
} catch (error) {
console.log(error);
}
};

const noteDate = (time) => {
return new Date(time*1000).toLocaleDateString();
};
const noteDate = (time) => {
return new Date(time * 1000).toLocaleDateString();
};

useEffect(() => {
getReviews();
}, []);
useEffect(() => {
getReviews();
}, []);

return (
<>
{reviews && <h5>what other users say about this book</h5>}
{reviews ? (reviews.map((note, index) => {
return (
<Container className='py-2' key={index}>
<NoteCard note={note} date={noteDate(note.date.seconds)} />
{reviews && <h5>what other users say about this book</h5>}
{reviews ? (
reviews.map((note, index) => {
return (
<Container className="py-2" key={index}>
<NoteCard note={note} date={noteDate(note.date.seconds)} />
</Container>
)})) :
<div className='py-3'>
<p>Loading reviews...</p>
</div>
}
);
})
) : (
<div className="py-3">
<p>Loading reviews...</p>
</div>
)}
</>
)
);
}

export default BookReviews
export default BookReviews;
Loading