From 88938d87d09dd1bd82bdaf71ce04f3aa13cd1809 Mon Sep 17 00:00:00 2001
From: PuffCab <berlinkollner@gmail.com>
Date: Wed, 6 Jul 2022 12:57:36 +0200
Subject: [PATCH] Code review Ottavia and Raul

---
 src/components/Chat.js         | 2 +-
 src/components/Details.js      | 9 +++++++--
 src/components/FooterFilter.js | 1 +
 src/components/Map.js          | 2 +-
 src/context/BookmarkContext.js | 2 +-
 src/context/MyContext.js       | 3 +++
 6 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/components/Chat.js b/src/components/Chat.js
index 00e6134..7acb86a 100644
--- a/src/components/Chat.js
+++ b/src/components/Chat.js
@@ -3,7 +3,7 @@ import { Box, Grid, Typography } from "@mui/material";
 import ChatMessageLeft from "./ChatMessageLeft";
 import ChatMessageRight from "./ChatMessageRight";
 import { myContext } from "../context/MyContext";
-
+//TODO as a general message, it would be nicer to have user's messages on the leftside (if you are a righty :P ) and the rest of the messages on the right side. You probably tried already, so this would be a nice addition.
 export default function Chat() {
   const { messages, documentID } = useContext(myContext);
 
diff --git a/src/components/Details.js b/src/components/Details.js
index 1e428cf..e43ba41 100644
--- a/src/components/Details.js
+++ b/src/components/Details.js
@@ -11,6 +11,8 @@ import MapButton from "./MapButton";
 import { doc, setDoc, deleteDoc, getDoc } from "firebase/firestore";
 import { db } from "../config";
 
+//TODO as a general comment, this component got pretty big, and there are some checks that are repeated , giving us the feeling that maybe splitting them into smaller components might be a good way to make it shorter and more readable.
+
 export default function Details() {
   const { status, user } = useContext(AuthContext);
   const [checked, setChecked] = useState(false);
@@ -30,7 +32,7 @@ export default function Details() {
   };
 
   // * Loop over deSubject JSON object with nested arrays to catch all subject entries
-  let recordSubjects = [];
+  let recordSubjects = []; //TODO maybe move the array inside the function since you are not directly using it later. Or Turn it into a State Variable that you set inside getSubjects()
   const getSubjects = (itemRecord) => {
     let subjects = Object.entries(itemRecord.object.proxies[1].dcSubject);
 
@@ -64,7 +66,7 @@ export default function Details() {
     return itemID;
   };
 
-  let finalID = id();
+  let finalID = id(); //TODO instead of return itemID you could a) create a state variable, and b) set it in id()
 
   const addBookmark = async () => {
     try {
@@ -156,6 +158,9 @@ export default function Details() {
                 {typeof itemRecord.object.proxies[1].dcTitle == "undefined"
                   ? location.state.element.title
                   : itemRecord.object.proxies[1].dcTitle.de}
+
+                {/*//TODO I (Raul) do not really understand that check with
+                typeof...but if it works...  */}
               </Typography>
               <Typography
                 paragraph
diff --git a/src/components/FooterFilter.js b/src/components/FooterFilter.js
index 93a6c01..2868aa2 100644
--- a/src/components/FooterFilter.js
+++ b/src/components/FooterFilter.js
@@ -23,6 +23,7 @@ import CheckIcon from "@mui/icons-material/Check";
 import { useNavigate } from "react-router-dom";
 import { myContext } from "../context/MyContext";
 import { AuthContext } from "../context/AuthContext";
+//TODO the filter function needs to be fixed
 
 export default function Footer() {
   const [show, setShow] = useState(false);
diff --git a/src/components/Map.js b/src/components/Map.js
index 1a7878d..f30a649 100644
--- a/src/components/Map.js
+++ b/src/components/Map.js
@@ -5,7 +5,7 @@ import { myContext } from "../context/MyContext";
 import Typography from "@mui/material/Typography";
 import { Box } from "@mui/material";
 import PersonIcon from "@mui/icons-material/Person";
-
+//TODO why is this function anonymous?
 export default function () {
   const location = useLocation();
   const { user } = useContext(myContext);
diff --git a/src/context/BookmarkContext.js b/src/context/BookmarkContext.js
index aa38946..0f3114f 100644
--- a/src/context/BookmarkContext.js
+++ b/src/context/BookmarkContext.js
@@ -1,7 +1,7 @@
 import { createContext, useState } from "react";
 
 export const BookmarkContext = createContext();
-
+//TODO are you really using it? If not, better remove it , unless you have something planned for it.
 export const BookmarkContextProvider = (props) => {
   return (
     <>
diff --git a/src/context/MyContext.js b/src/context/MyContext.js
index 27a98f3..cef6d81 100644
--- a/src/context/MyContext.js
+++ b/src/context/MyContext.js
@@ -15,6 +15,8 @@ import {
 
 export const myContext = createContext();
 
+//TODO as a general comment, this context might be splitted into two, one for the search and one for the comments, to make it shorter
+
 export const MyContextProvider = (props) => {
   const [test, setTest] = useState({ name: "John", password: "tomato" });
 
@@ -68,6 +70,7 @@ export const MyContextProvider = (props) => {
     inputField.addEventListener("keyup", (event) => {
       if (event.key === "Enter") {
         let value = document.querySelector("#userInputValue").value;
+        //TODO the comparison below isn't working
         if (value != "") {
           setUserInput(value);
           setLoading(true);