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

Jasper #2

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

Jasper #2

wants to merge 35 commits into from

Conversation

jasperteo
Copy link

No description provided.

@@ -1,51 +1,71 @@
import { useState, useEffect } from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer browserRouter implementations over hash routers, we discussed how you can fix this in the morning standup sessions

function App() {
const [messages, setMessages] = useState([]);
export default function App() {
//check login status
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work lifting up the required states for the application to run smoothly

let messageListItems = messages.map((message) => (
<li key={message.key}>{message.val}</li>
));
const router = createHashRouter([
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use nested Routing along with an outlet to reduce the number of times you load and reload the navbar

@@ -0,0 +1,92 @@
import { useState } from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work keeping all auth logic in one component and either showing the form to login or the button ton sign out, really smart UI and UX thought!

@@ -1,6 +1,10 @@
import React from "react";
import ReactDOM from "react-dom/client";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove strict mode!

@@ -1,21 +1,27 @@
// Import the functions you need from the SDKs you need
Copy link
Contributor

Choose a reason for hiding this comment

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

Get conventional setup here!


const messagesRef = databaseRef(database, DB_MESSAGES_KEY);

const writeData = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice implementation of async and await code!


return (
<form onSubmit={(e) => (e.preventDefault(), e.target.reset())}>
<input
Copy link
Contributor

Choose a reason for hiding this comment

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

Use some stylesheets to reduce repeated code

url = await getDownloadURL(newStorageRef);
name = file.name;
}
set(push(messagesRef), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great implementation of likes to ensure that users can only like once, I also like that you have a like Count such that you dont need to process firebase data every load.

@@ -0,0 +1,167 @@
import { useEffect, useState } from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

Great structure and logic implemented here! Superb work

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