Marina's project 1: My shopping list app #43
Conversation
|
|
||
| ## Requirements | ||
|
|
||
| To install and launch the app, you will need NodeJS v16+ |
There was a problem hiding this comment.
You could also tell the reader how to install node
src/App.js
Outdated
| <Container className="App mt-5 col-11 col-md-6 col-lg-5 align-items-center"> | ||
| <Stack gap={2}> | ||
| <div className="header">My shopping list</div> | ||
| <div> |
There was a problem hiding this comment.
Since ItemsList also has a <div> as wrapper, do we need this one?
| <div> | ||
| <Container className="col-12 col-lg-11 text-center"> | ||
| <InputGroup id="main-input-field" className="mb-3 mt-4"> | ||
| <Form.Control |
There was a problem hiding this comment.
Not 100% sure here, but do we need to use a Form element here, when we actually are not using a form? Wouldn't a normal input suffice, to which we can apply bootstrap classes?
src/Components/ItemsList.js
Outdated
| //We need to be able to add items to the list | ||
| //Items are added in the UserInput component: when user inputs something into the input field and then clicks "add item" , the item should get added into the itemsArray | ||
| //Each item can be deleted | ||
| //Each item can be changed | ||
| //Each item can be edited | ||
| //Each item has checkbox, once the user got the item, they may tick the checkbox and the item will get crossed out |
There was a problem hiding this comment.
this is already documented in the README and shouldn't live in our code :)
src/Components/ItemsList.js
Outdated
| //Each item can be edited | ||
| //Each item has checkbox, once the user got the item, they may tick the checkbox and the item will get crossed out | ||
|
|
||
| //1) adding items logic |
There was a problem hiding this comment.
Redundant comment, as the function name already suggests what is being done here
|
|
||
| //4) crossing items out logic | ||
| //this function is passed as props to Items | ||
| checkItem = (key) => { |
There was a problem hiding this comment.
If comment says crosing out, but function says checking, what is it? crossing or checking? Name accordingly I would advise :)!
src/Components/ItemsList.js
Outdated
| if (item.key === key) { | ||
| item.isChecked = !item.isChecked; | ||
| } | ||
| return { ...item }; |
src/Components/ItemsList.js
Outdated
| this.setState({ itemsList: itemsList }); | ||
| localStorage.setItem("itemsList", JSON.stringify(itemsList)); | ||
| }; | ||
| //5) logic to clear all items |
| import Row from "react-bootstrap/Row"; | ||
| import Col from "react-bootstrap/Col"; | ||
| import Form from "react-bootstrap/Form"; | ||
| export default class Items extends React.Component { |
There was a problem hiding this comment.
This component is describing how a single Item looks like. I would recommend naming it in singular because of that. Item instead of Items
| import Form from "react-bootstrap/Form"; | ||
| export default class Items extends React.Component { | ||
| render() { | ||
| const items = this.props.itemsList.map((item, key) => { |
There was a problem hiding this comment.
If I compare ItemsList with Items, I don't see how the list is an actual list. You are rendering the list in this component. To correct this, I would run this map in the ItemsList component and pass down the updater etc. functions to the Item component, within the map, which will then determine how the updating/deletion etc. works
There was a problem hiding this comment.
in ItemsList it should look like this:
render() {
return {this.itemsList.map((item, key) => {
return <Item item={item} key={key} />
}}
}
|
|
||
| ## Requirements | ||
|
|
||
| To install and launch the app, you will need NodeJS v16+ |
There was a problem hiding this comment.
You could also tell the reader how to install node
src/App.css
Outdated
| /*max-width: 400px; | ||
| min-width: 340px; | ||
| margin: 40px auto; */ |
| import Form from "react-bootstrap/Form"; | ||
| export default class Items extends React.Component { | ||
| render() { | ||
| const items = this.props.itemsList.map((item, key) => { |
There was a problem hiding this comment.
in ItemsList it should look like this:
render() {
return {this.itemsList.map((item, key) => {
return <Item item={item} key={key} />
}}
}
| ); | ||
| }); | ||
|
|
||
| return <div>{items}</div>; |
There was a problem hiding this comment.
Here you are wrapping the items in another div again, not sure if that is desired or not
src/Components/ItemsList.js
Outdated
| if (item.key === key) { | ||
| item.name = name; | ||
| } | ||
| return { ...item }; |
There was a problem hiding this comment.
| return { ...item }; | |
| return item; |
This is the same thing
No description provided.