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

Code review #1

Open
wants to merge 39 commits into
base: initial
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
4b2c95a
Adding base structure to app. Installing sass
1n3ffbl3 Apr 17, 2019
82aa427
Adding styles to list component. Creating modal component
1n3ffbl3 Apr 17, 2019
0ae7a3e
Adding form to modal
1n3ffbl3 Apr 17, 2019
43ea61f
Adding header component. Installing react-icons
1n3ffbl3 Apr 17, 2019
2c78e42
Adding function to close modal
1n3ffbl3 Apr 17, 2019
8bfa08d
Adding style to modal and close button
1n3ffbl3 Apr 17, 2019
5540110
Using Route from react-router-dom. Adding and styling HeaderNav compo…
1n3ffbl3 Apr 17, 2019
332db5d
Adding function to open modal
1n3ffbl3 Apr 17, 2019
73da11c
Changing Form to stateful component. Adding addItem and handleInputCh…
1n3ffbl3 Apr 17, 2019
6b5f5c4
Fixing error with addItem. Using context API
1n3ffbl3 Apr 17, 2019
ccef892
Adding style to list item
1n3ffbl3 Apr 17, 2019
fb683ac
Adding style to button
1n3ffbl3 Apr 17, 2019
a023d8d
Trying to add search feature
1n3ffbl3 Apr 18, 2019
3e46fb8
Creating Input component
1n3ffbl3 Apr 18, 2019
34ae713
Fixing search. Fixing label on Quantity. Aligning content to center. …
1n3ffbl3 Apr 18, 2019
2dfce07
Fixing modal size
1n3ffbl3 Apr 18, 2019
4a25c4e
CSS Work in progress
1n3ffbl3 Apr 18, 2019
a6a6c3e
Improving Checkbox. Adding marked as completed items to right list.
1n3ffbl3 Apr 18, 2019
d05d519
Adding logic to update price and buyer.
1n3ffbl3 Apr 18, 2019
9f8afdf
Adding icon
1n3ffbl3 Apr 18, 2019
100b87f
Adding style to item
1n3ffbl3 Apr 19, 2019
5a5d83e
Code cleanup
1n3ffbl3 Apr 19, 2019
1b5afc9
Adding delete item function
1n3ffbl3 Apr 19, 2019
074e3f8
CSS Work in progress
1n3ffbl3 Apr 19, 2019
8b98f24
Adding recipes and stats views
1n3ffbl3 Apr 19, 2019
a61c912
Refactoring + CSS WIP
1n3ffbl3 Apr 19, 2019
6e798c5
Refactoring from state to localStorage.
1n3ffbl3 Apr 19, 2019
9bce464
Adding search by category
1n3ffbl3 Apr 19, 2019
5d97413
Adding working dropdown for category
1n3ffbl3 Apr 19, 2019
eac76f5
CSS WIP responsive UI
1n3ffbl3 Apr 19, 2019
8bfd8b1
CSS WIP
1n3ffbl3 Apr 19, 2019
81f162b
CSS WIP
1n3ffbl3 Apr 19, 2019
71782c0
CSS WIP
1n3ffbl3 Apr 19, 2019
80ccbef
Clearing up code
1n3ffbl3 Apr 20, 2019
f4ff126
Cleaning up code. CSS WIP
1n3ffbl3 Apr 20, 2019
785a13a
Removing App.test
1n3ffbl3 Apr 20, 2019
351f70e
Adding propTypes
1n3ffbl3 Apr 20, 2019
7ffb4bc
Clearing index.html. Adding now.json for deployment. ignoring build/.
1n3ffbl3 Apr 20, 2019
f5e86c3
Adding README
1n3ffbl3 Apr 20, 2019
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,5 @@ typings/

# next.js build output
.next

build/
80 changes: 79 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1 +1,79 @@
# Groceries-App
# Groceries App

App which allows you to create a list of items to buy.

Checkout here the app: https://groceries-app.mizgierm.now.sh/

### Current project status:

1. You can:
- add new item to the shopping list and specify its name and quantity
- delete an item, if you made a mistake by adding it to the list
- browse through the shopping list of items to buy and already bought ones
- mark an item as bought and specify its cost and who bought it
- search for your items by name or category (vegetable, fruit, meat, bakery)

2. Next release (v2):
- view some kind of statistics (e.g. how much money was spent this month, how much each user spent, ...)
- recipe page contains list of recipes. It is possible to click on recipe and add all its items to 'to buy' list
- app suggests recipes based on your shopping list
- possibility to add all recipe ingredients to shopping list
- fixing potential bugs

3. Next release (v3):
- remainders can be added to specific items (e.g. using google calendar API)

### How to run

Clone repository on your computer
```
git clone https://github.com/1n3ffbl3/Groceries-App.git
```

Ensure that you have either npm or yarn installed.

Run 'yarn' to install dependencies and 'yarn start' to start the application.
```
yarn
yarn start
```

Application is available on localhost:3000

### Technical aspects

The app use localStorage to store data. If you run the app on your browser in multiple tabs, it will share data between those tabs after actions on each tab (adding new item / marking as bought / deleting an item).

It is worth to checkout 'GroceryService' file to see, how above operations are done.

### Supported cases for multiple users

- User1 will be aliased as U1
- User2 will be aliased as U2

I assume that U1 and U2 are using same computer and browser, but different tabs.

- U1 adds an item to 'to buy' list. For U2 change is visible only after:
- refresh of the page
- adding new item (adding an item fetches all items from storage)
- U1 updates an item and marks it as completed. U2 sees change only after:
- refresh of the page
- add new item
- deletes any existing 'to buy' item (which is not this one)
- U1 deletes an item. U2 sees change only after:
- refresh of the page
- add new item
- deletes any existing 'to buy' item (which is not this one)

Case when U1 marks item as completed, but didn't finish filling in information
(price and buyer) and in the same time U2 deletes same item, is currently not supported.

### To be proud

- using sass in efficient way (reducing redundant css)
- using localStorage to save data inside browser to support multiple users
- reducing redundant code by making reusable components
- effectively using React Context API, which is a lighter alternative to redux, which makes app less complex
- finishing first version of application in specified time (3.5 days)
with all features of first release
- fixing unexpected bugs
52 changes: 52 additions & 0 deletions now.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
{
"version": 2,
"name": "groceries-app",
"builds": [
{
"src": "package.json",
"use": "@now/static-build",
"config": {
"distDir": "build"
}
}
],
"routes": [
{
"src": "/static/(.*)",
"headers": {
"cache-control": "s-maxage=31536000,immutable"
},
"dest": "/static/$1"
},
{
"src": "/favicon.ico",
"dest": "/favicon.ico"
},
{
"src": "/asset-manifest.json",
"dest": "/asset-manifest.json"
},
{
"src": "/manifest.json",
"dest": "/manifest.json"
},
{
"src": "/precache-manifest.(.*)",
"dest": "/precache-manifest.$1"
},
{
"src": "/service-worker.js",
"headers": {
"cache-control": "s-maxage=0"
},
"dest": "/service-worker.js"
},
{
"src": "/(.*)",
"headers": {
"cache-control": "s-maxage=0"
},
"dest": "/index.html"
}
]
}
33 changes: 33 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"name": "groceries-app-react",
Copy link
Author

Choose a reason for hiding this comment

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

We use spaces instead of tabs for indenting code. Specifically, two spaces.

"version": "0.1.0",
"private": true,
"dependencies": {
"node-sass": "^4.11.0",
"react": "^16.8.6",
"react-dom": "^16.8.6",
"react-icons": "^3.6.1",
"react-router-dom": "^5.0.0",
"react-scripts": "2.1.8"
},
"scripts": {
"start": "react-scripts start",
"build": "react-scripts build",
"test": "react-scripts test",
"eject": "react-scripts eject",
"now-build": "react-scripts build"
},
"eslintConfig": {
"extends": "react-app"
},
"browserslist": [
">0.2%",
"not dead",
"not ie <= 11",
"not op_mini all"
],
"devDependencies": {
"bootstrap": "^4.3.1",
"reactstrap": "^8.0.0"
}
}
Binary file added public/favicon.ico
Binary file not shown.
18 changes: 18 additions & 0 deletions public/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<!DOCTYPE html>
<html lang="en">

Copy link
Author

Choose a reason for hiding this comment

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

Unnecessary space

<head>
<meta charset="utf-8" />
<link rel="shortcut icon" href="./favicon.ico" />
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no" />
<meta name="theme-color" content="#000000" />
<link rel="manifest" href="./manifest.json" />
<title>Groceries App</title>
</head>

<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>
</body>

</html>
15 changes: 15 additions & 0 deletions public/manifest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"short_name": "React App",
"name": "Create React App Sample",
"icons": [
{
"src": "favicon.ico",
"sizes": "64x64 32x32 24x24 16x16",
"type": "image/x-icon"
}
],
"start_url": ".",
"display": "standalone",
"theme_color": "#000000",
"background_color": "#ffffff"
}
24 changes: 24 additions & 0 deletions src/components/Button/Button.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react';
import PropTypes from 'prop-types';
import styles from './Button.module.scss';

const Button = ({ children, secondary, ...props }) => {
const buttonClass = secondary ? styles.secondary : styles.button;
return (
<>
Copy link
Author

Choose a reason for hiding this comment

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

Use <Fragment></Fragment> instead of <></>. That way it looks more consistent with the rest of the code.

<button className={buttonClass} {...props}>
{children}
</button>
</>
);
};

Button.propTypes = {
secondary: PropTypes.bool,
};

Button.defaultProps = {
secondary: null,
Copy link
Author

Choose a reason for hiding this comment

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

Only define defaultProps when they're needed. In this case, setting secondary to null doesn't change anything. Treat defaultProps as a way of telling the users, "Hey, I really need a value for this in order to work correctly"

Also, defaultProps should correctly reflect the expected type according to propTypes. In this case, a boolean.

};

export default Button;
18 changes: 18 additions & 0 deletions src/components/Button/Button.module.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
.button {
font-size: 10px;
text-decoration: none;
padding: 7px 12px;
Copy link
Author

Choose a reason for hiding this comment

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

Usually for padding, margin, font-size, color, etc... it's better to have a variables file with values used across the whole app and then use those variables instead of the hardcoded values. That makes the UI more consistent, easier to modify and easier to maintain.

font-weight: 500;
background: rgb(0, 83, 135);
border: 2px solid white;
color: white;
Copy link
Author

Choose a reason for hiding this comment

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

Consistent indentation

margin-bottom: 24px;
}

.secondary {
@extend .button;
background-color: white;
border: 2px solid rgb(0, 83, 135);
color: rgb(0, 83, 135);
font-size: 14px;
}
32 changes: 32 additions & 0 deletions src/components/Checkbox/Checkbox.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React from 'react';
import PropTypes from 'prop-types';
import styles from './Checkbox.module.scss';

const Checkbox = ({ type, name, checked, disabled, onChange }) => (
Copy link
Author

Choose a reason for hiding this comment

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

We never use this syntax of implicit return. Better to be explicit and use:

const Checkbox = () => {
  return ();
}

For various reasons, first, always favor explicit behavior over implicit (what's obvious for you now, might not be that obvious for you two months after) also, most of the time, you end up doing something before the return prop so you don't have to end up modifying a lot of lines (cleaner git diffs) and finally, it gives every React component a recognizable shape.

<label className={styles.container}>
<input
type={type}
name={name}
checked={checked}
onChange={onChange}
disabled={disabled}
/>
<span className={styles.checkmark}></span>
</label>
);

Checkbox.propTypes = {
type: PropTypes.string,
Copy link
Author

Choose a reason for hiding this comment

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

type shouldn't be a prop. It won't make sense to have something like <Checkbox type="text" />. When designing interfaces (APIs), always try to limit as much as you can or the users will find a way to mess things up.

name: PropTypes.string.isRequired,
checked: PropTypes.bool,
onChange: PropTypes.func.isRequired,
disabled: PropTypes.bool,
};

Checkbox.defaultProps = {
type: 'checkbox',
checked: false,
disabled: false,
};

export default Checkbox;
62 changes: 62 additions & 0 deletions src/components/Checkbox/Checkbox.module.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
.container {
position: relative;
cursor: pointer;
-webkit-user-select: none;
Copy link
Author

Choose a reason for hiding this comment

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

Don't manually auto-prefix. Leave that to the tooling.

}

.container input {
Copy link
Author

Choose a reason for hiding this comment

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

Since SASS it's being used, take advantage of nesting.

position: absolute;
opacity: 0;
cursor: pointer;
height: 0;
width: 0;
}

.checkmark {
position: absolute;
height: 40px;
width: 40px;
background-color: #eee;

@media (max-width: 600px) {
height: 30px;
width: 30px;
top: 10px;
left: 15px;
}
}

.container:hover input ~ .checkmark {
Copy link
Author

Choose a reason for hiding this comment

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

Nesting

background-color: #ccc;
}

.container input:checked ~ .checkmark {
background-color: rgb(200, 235, 223);
}

.checkmark:after {
content: "";
position: absolute;
display: none;
}

.container input:checked ~ .checkmark:after {
display: block;
}

.container .checkmark:after {
left: 9px;
top: 5px;
width: 20px;
height: 25px;
border: solid white;
border-width: 0 3px 3px 0;
-webkit-transform: rotate(45deg);
-ms-transform: rotate(45deg);
transform: rotate(45deg);

@media (max-width: 600px) {
height: 15px;
width: 10px;
}
}
Loading