Cleaned up review branch #1

MLewiDev · 2021-02-12T14:19:00Z

Pull request for review purposes.

MLewiDev · 2021-02-12T14:22:48Z

README.md

@@ -1,58 +0,0 @@
-# Photo Album sites.


Nice readme, some English mistakes, you could try to use some VS code extension for grammar.

MLewiDev · 2021-02-12T20:18:45Z

src/views/Footer.js

-        </p>
-        <p className="mb-1">Photo Web Album - Here you can collect yours photos. You can gropued by category, tags ...</p>
-        <p className="mb-0">
-          Maid by Maciej using Bootstrap ver 5.0 &nbsp; &nbsp; 


nice one ;). "Pokojówka Macieja korzystająca z Bootstrap w wersji 5.0"

MLewiDev · 2021-02-12T20:32:21Z

src/App.css

@@ -1,38 +0,0 @@
-.App {


Are styles defined in this file used?

MLewiDev · 2021-02-12T20:32:52Z

src/App.js

@@ -1,42 +0,0 @@
-import './App.css';


Is this import needed?

MLewiDev · 2021-02-12T20:33:32Z

src/App.test.js

@@ -1,8 +0,0 @@
-import { render, screen } from '@testing-library/react';


I think you can remove this file.

MLewiDev · 2021-02-12T20:36:08Z

public/index.html

-
-    <link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-giJF6kkoqNQ00vy+HMDP7azOuL0xtbfIcaT9wjKHr8RbDVddVHyTfAAsrekwKmP1" crossorigin="anonymous">
-
-    <style>


Do we need this as inline style or can it also be placed in style.scss?

MLewiDev · 2021-02-12T20:41:08Z

src/assets/scss/style.scss

-    border-width: 0px;
-    text-transform: lowercase;
-  }
-}


It's good practice to have newline at end o file, see: https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline#:~:text=If%20content%20is%20added%20to,to%20include%20a%20newline%20character.

Also you can add some linting extensions for your IDE to check JS and styles code for consistent formatting and simple things like newlines for example.

MLewiDev · 2021-02-12T20:42:24Z

src/assets/scss/style.scss

-    background-color: $main-color;
-    color: #FFF;
-    font-size: 1rem;
-    border-radius: 50px;


Using more constants for default values of margin, padding, font-size etc. would be nice.

MLewiDev · 2021-02-12T20:42:51Z

src/components/Album.js

-import { Link } from "react-router-dom";
-import { timestamp } from './../firebase/config';
-import useComments from './../composables/useComments';
-// import getComments from './../composables/getComments';


remove unused code

MLewiDev · 2021-02-12T20:43:38Z

src/components/Album.js

-
-const Album = ({ title, description, imageUrl, createdAt, category, id }) => {
-
-  const timeSince = (date) => {


extract to some utility file.

MLewiDev · 2021-02-12T20:43:54Z

src/components/Album.js

-  const [comments, setComments] = useState([]);
-  const [comment, setComment] = useState([]);
-  const { error, addComment } = useComments('comments', id);
-  // const { comments } = getComments('comments', id);


remove unused

MLewiDev

The Good:

Good cohesive chunks of code
Good structure of application
Simple and easy to understand components
Properly extracted reusable parts
Good use of html and styling

Things to Improve:

More structure in styles
Some grammar issues, check if some extension could help you a little bit with that
Some linting could be improved for more consistency across files

Remarks:

Didn't check firebase related API calls
Testing isn't considered as scope of this task
I found some areas of app that could be improved like for example "enter on comment input works as reload of page", but they are considered as further improvements and not bugs.

Conclusion

Very nice code for a first attempt in React. It's readable and maintainable, properly structured with clean HTML. Only some minor things to improve from my side.

MLewiDev · 2021-02-12T21:20:01Z

src/components/Album.js

-      return Math.floor(interval) + " minutes ago";
-    }
-
-    if (seconds === 0) {


You can use here <= 0 as I saw a bug that in the beginning the time starts at minus values.

MLewiDev · 2021-02-13T07:09:38Z

src/views/Hero.js

-                }
-              </datalist>
-            </div>
-            {/* <input className="form-control me-2 search-bar" type="search" placeholder="Search" aria-label="Search" onChange={searchAlbum} /> */}


remove unused

MLewiDev · 2021-02-13T07:09:56Z

src/views/Hero.js

-   );
-}
-
-export default Hero;


MLewiDev · 2021-02-13T07:10:35Z

src/views/Hero.js

-      <div className="row">
-        <div className="col-lg-6 col-md-8 mx-auto">
-          <h1 className="fw-light">Photo Album</h1>
-          <p className="lead text-muted">Here you can collect yours photos. You can gropued by category, tags ... </p>


...your photos....grouped....

MLewiDev · 2021-02-13T07:12:00Z

src/views/Footer.js

-  }
-
-  return ( 
-


Why do you have sometimes whitelines here and sometimes not?

MLewiDev · 2021-02-13T07:34:41Z

src/composables/useStorage.js

-  const [error, setError] = useState(null);
-
-  const uploadImage = async (file) => {
-    let filePath = `images/${file.name}`;


MLewiDev · 2021-02-13T07:39:24Z

src/composables/getComments.js

@@ -1,29 +0,0 @@
-import { projectFirestore } from '../firebase/config'


why is it getComments and not useComments?

MLewiDev · 2021-02-13T07:40:58Z

src/composables/getCollection.js

@@ -1,38 +0,0 @@
-import { projectFirestore } from '../firebase/config'


why getCollection no useCollection?

MLewiDev · 2021-02-13T07:58:42Z

src/components/CreateAlbum.js

-      setFileError(null);
-    } else {
-      setFile(null);
-      setFileError('Wron type of file.');


MLewiDev · 2021-02-13T07:59:31Z

src/components/CreateAlbum.js

-                  <h2>Create New Album</h2>
-                  <form onSubmit={handleSubmit} className="php-email-form">
-                    <div className="form-group mt-3">
-                      <input


You could add some guidance which ones are required

Cleaned up review branch

7f79658

MLewiDev marked this pull request as draft February 12, 2021 14:19

MLewiDev commented Feb 12, 2021

View reviewed changes

MLewiDev commented Feb 13, 2021

View reviewed changes

		@@ -1,8 +0,0 @@
		import { render, screen } from '@testing-library/react';


		<link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-giJF6kkoqNQ00vy+HMDP7azOuL0xtbfIcaT9wjKHr8RbDVddVHyTfAAsrekwKmP1" crossorigin="anonymous">

		<style>


		const Album = ({ title, description, imageUrl, createdAt, category, id }) => {

		const timeSince = (date) => {

		@@ -1,29 +0,0 @@
		import { projectFirestore } from '../firebase/config'

		@@ -1,38 +0,0 @@
		import { projectFirestore } from '../firebase/config'

Cleaned up review branch #1

Are you sure you want to change the base?

Cleaned up review branch #1

Uh oh!

Conversation

MLewiDev commented Feb 12, 2021

Uh oh!

MLewiDev Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MLewiDev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MLewiDev Feb 12, 2021 •

edited

Loading