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

Angry happy note #40

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

Conversation

johannacatalinismith
Copy link

@johannacatalinismith johannacatalinismith commented Jun 13, 2024

Copy link

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

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

Dear Johanna, congratulations for completing your final project on your own! 🥳

You delivered a simple yet working app that followed our guidelines and meets all the requirements. Good job exploring new technologies like react-google-maps and further deepen your knowledge in web development. Now that said, the UI is kept to the minimum, the use of CSS was limited and relied mostly on displaying the map, that's a pity considering all the CSS libraries and projects explored during the bootcamp. The routes are there, even if there is really no "multiple-pages effect", opening the modal to write a note simulates a new location in the app even though there is no really a new full page implemented. Overall it was a very simple project that could have been developed even further in its details, like for examples having more data populated in the DB to display some other notes, right now I could only test an empty map and my newly added note.
I would like to push you to do even more and I hope you will find the time and will to continue improving your app that starts with a cool concept and can be the base to something even better!

To pass I'll ask you to deploy your routes adding a netlify.tomlfile like showed during class, and please clean up and refactor the code based on my suggestions left in the review. Thank you and again, big congrats! ⭐

.then((data) => setNotes(data));
// fetch more notes, every time you change page
}, [pathname]);
console.log(notes);

Choose a reason for hiding this comment

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

please remove development console.log() 🧹

Choose a reason for hiding this comment

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

Your code is correct but readability can be improved, take a look at this refactored code:

import { useEffect, useState } from "react";
import {
  createBrowserRouter,
  Link,
  Outlet,
  RouterProvider,
  useLocation,
} from "react-router-dom";
import { APIProvider, Map } from "@vis.gl/react-google-maps";
import { Note } from "./Note";
import { HomePage } from "./HomePage";
import { FormPage } from "./FormPage";
import { LocationContext } from "./LocationContext";
import "./App.css";

const sweden = { lat: 62.3875, lng: 16.325556 };

const AppLayout = () => {
  const { pathname } = useLocation();
  const [notes, setNotes] = useState([]);
  const [location, setLocation] = useState(sweden);
  const [noteId, setNoteId] = useState("");

  useEffect(() => {
    const url = `${import.meta.env.VITE_BACKEND_URL}/notes`;
    fetch(url)
      .then((response) => response.json())
      .then((data) => setNotes(data));
  }, [pathname]);

  useEffect(() => {
    if (pathname === "/new") {
      navigator.geolocation.getCurrentPosition((position) => {
        setLocation({
          lat: position.coords.latitude,
          lng: position.coords.longitude,
        });
      });
    }
  }, [pathname]);

  return (
    <div className="App">
      <header className="App-header">
        <h1 className="App-title">Angry happy note</h1>
        <ul className="App-menu">
          <li>
            <Link to="/" unstable_viewTransition>
              Note map
            </Link>
          </li>
          <li>
            <Link to="/new" unstable_viewTransition>
              Add note
            </Link>
          </li>
        </ul>
      </header>

      <APIProvider apiKey={import.meta.env.VITE_GOOGLE_MAPS_API_KEY}>
        <Map
          className="App-map"
          defaultCenter={sweden}
          defaultZoom={5}
          center={pathname === "/new" && location.lat && location}
          zoom={pathname === "/new" && location.lat !== sweden.lat && 16}
          gestureHandling="greedy"
          disableDefaultUI
          mapId="happyangrynote"
        >
          {pathname === "/" &&
            notes.map((note) => (
              <Note
                key={note._id}
                {...note}
                open={note._id === noteId}
                onClick={() => setNoteId(note._id === noteId ? "" : note._id)}
              />
            ))}
        </Map>
      </APIProvider>

      <div className="App-outlet">
        <LocationContext.Provider value={location}>
          <Outlet />
        </LocationContext.Provider>
      </div>
    </div>
  );
};

const router = createBrowserRouter([
  {
    path: "/",
    element: <AppLayout />,
    children: [
      {
        index: true,
        element: <HomePage />,
      },
      {
        path: "new",
        element: <FormPage />,
      },
    ],
  },
]);

export const App = () => (
  <RouterProvider
    router={router}
    future={{
      v7_startTransition: true,
    }}
  />
);

Choose a reason for hiding this comment

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

All your components can be grouped under a folder called componentsas showed during class, in which you can put the .jsx and .css file for such component 👍

Choose a reason for hiding this comment

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

what is this empty component representing?

Choose a reason for hiding this comment

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

please clean up the code from empty files

@johannacatalinismith
Copy link
Author

Hey! Thanks for your feedback. My plan is to now, when im no longer on a weekly deadline, that is combine with full time work and parenting, are going to work on building my web-developer portfolio :) Im going to use projects from Technigo and do the stretch goals! Have done all the changes you required.

Copy link

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

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

Thank you for the updates, I left a couple of comments after a second review, please edit the code once more, thank you :)

Choose a reason for hiding this comment

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

App.jsx and App.css should be outside the components folder, please see our class projects structure -> https://github.com/Technigo/Web-Spring-24/tree/main/weeks/week-5/explanations/class-examples/planets-react/src

Comment on lines 63 to 92
<APIProvider apiKey={import.meta.env.VITE_GOOGLE_MAPS_API_KEY}>
<Map
// here we choose from where on the map we want to start
className="App-map"
defaultCenter={sweden}
defaultZoom={4}
center={pathname === "/new" && location.lat && location}
zoom={pathname === "/new" && location.lat !== sweden.lat && 16}
gestureHandling="greedy"
disableDefaultUI
mapId="happyangrynote"
>
{pathname === "/" &&
// this is so the user can open one note at a time
notes.map((note) => (
<Note
key={note._id}
{...note}
open={note._id == noteId}
onClick={() => {
if (note._id === noteId) {
setNoteId("");
} else {
setNoteId(note._id);
}
}}
/>
))}
</Map>
</APIProvider>

Choose a reason for hiding this comment

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

why not extracting a Map component here?

// here we choose from where on the map we want to start
className="App-map"
defaultCenter={sweden}
defaultZoom={4}

Choose a reason for hiding this comment

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

why this value defaultZoom={4}? Would be best to avoid the world map to repeat multiple times when zooming out 👍

@johannacatalinismith
Copy link
Author

Done the requested changes :)

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