-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: initial
Are you sure you want to change the base?
Code review #1
Changes from all commits
4b2c95a
82aa427
0ae7a3e
43ea61f
2c78e42
8bfa08d
5540110
332db5d
73da11c
6b5f5c4
ccef892
fb683ac
a023d8d
3e46fb8
34ae713
2dfce07
4a25c4e
a6a6c3e
d05d519
9f8afdf
100b87f
5a5d83e
1b5afc9
074e3f8
8b98f24
a61c912
6e798c5
9bce464
5d97413
eac76f5
8bfd8b1
81f162b
71782c0
80ccbef
f4ff126
785a13a
351f70e
7ffb4bc
f5e86c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,3 +59,5 @@ typings/ | |
|
||
# next.js build output | ||
.next | ||
|
||
build/ |
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 |
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" | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
{ | ||
"name": "groceries-app-react", | ||
"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" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<!DOCTYPE html> | ||
<html lang="en"> | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> |
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" | ||
} |
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 ( | ||
<> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
<button className={buttonClass} {...props}> | ||
{children} | ||
</button> | ||
</> | ||
); | ||
}; | ||
|
||
Button.propTypes = { | ||
secondary: PropTypes.bool, | ||
}; | ||
|
||
Button.defaultProps = { | ||
secondary: null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only define Also, |
||
}; | ||
|
||
export default Button; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
.button { | ||
font-size: 10px; | ||
text-decoration: none; | ||
padding: 7px 12px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually for |
||
font-weight: 500; | ||
background: rgb(0, 83, 135); | ||
border: 2px solid white; | ||
color: white; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
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 }) => ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
.container { | ||
position: relative; | ||
cursor: pointer; | ||
-webkit-user-select: none; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't manually auto-prefix. Leave that to the tooling. |
||
} | ||
|
||
.container input { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since |
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
There was a problem hiding this comment.
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.