Conversation
Lesson 2 Contact List
Tweaked style, finished search bar, added remove button and props val…
ivome
left a comment
ivome
left a comment
There was a problem hiding this comment.
Great job on the implementation!
You might be interested in a CSS coding style guide:
https://smacss.com/
src/Contact.js
Outdated
| occupation: PropTypes.string.isRequired, | ||
| onRemove: PropTypes.function.isRequired, | ||
| _id: PropTypes.number.isRequired | ||
| }; |
There was a problem hiding this comment.
Great job with the PropTypes!
There was a problem hiding this comment.
I had been confused on why I was getting PropTypes linter errors, looking at your code helped me understand how to validate them, thank you again :D
src/App.js
Outdated
|
|
||
| handleSearchBarChange(event) { | ||
| this.setState({ | ||
| contacts: this.state.contacts, |
There was a problem hiding this comment.
This line is not needed, since you are reassigning the same value.
chrisperk
left a comment
chrisperk
left a comment
There was a problem hiding this comment.
Great stuff, Alex! I learned a lot reviewing your code, thank you! And I didn't see any errors.
src/App.js
Outdated
| import ContactList from './ContactList'; | ||
| import SearchBar from './SearchBar.js'; | ||
|
|
||
| /* eslint max-len: [1, {"ignoreUrls": true}] */ |
There was a problem hiding this comment.
Great find, thank you for sharing this rule!
src/Contact.js
Outdated
| occupation: PropTypes.string.isRequired, | ||
| onRemove: PropTypes.function.isRequired, | ||
| _id: PropTypes.number.isRequired | ||
| }; |
There was a problem hiding this comment.
I had been confused on why I was getting PropTypes linter errors, looking at your code helped me understand how to validate them, thank you again :D
… reset button, unselect button, cool new styling, the works!
…gorithm to reflect multiple hits
src/SelectedContactList.js
Outdated
|
|
||
| return ( | ||
| <div className="selected-contact"> | ||
| <h1>{props.contacts.length > 0 ? 'Selected contacts' : 'No contacts selected'}</h1> |
There was a problem hiding this comment.
This is really cool! I was trying to figure out how to do this for like hours.
… action expiration
michaeb
left a comment
michaeb
left a comment
There was a problem hiding this comment.
Hi Alex, I like how you changed some of the components to functional ones. I also looked through and it looks like you're careful not to mutate your state too, nicely done.
ivome
left a comment
ivome
left a comment
There was a problem hiding this comment.
Really great implementation!! Clean code and great functionality. Keep it up!!
| @@ -0,0 +1,54 @@ | |||
| import React, { Component, PropTypes } from 'react'; | |||
|
|
|||
| class Action extends Component { | |||
There was a problem hiding this comment.
I'd be careful with the naming when using such words. An action usually refers to an actual action in software. So maybe ActionHistoryItem would be more descriptive.
| return ( | ||
| <div className="side-bar action-history"> | ||
| <h3>Action History</h3> | ||
| {props.actions.length > 0 ? null : 'Perform an action, jagweed.'} |
There was a problem hiding this comment.
When checking if an array has items, you can use the shorthand and just write props.actions.length ? null : '...' Since length returns a truthy value when there is items in the array.
| {props.actions.length > 0 ? null : 'Perform an action, jagweed.'} | ||
| <div className="side-bar-scroll"> | ||
| <ul className="action-list"> | ||
| {props.actions.slice(0, 10).map(action => { |
There was a problem hiding this comment.
Instead of keeping all history items in memory, you could also implement the limit when adding items to the queue. If you have more than 10 items right now and then remove some, the old items reappear. Since we have the expiration time, it's not that big of a deal, otherwise you would have a memory leak in your application and that action history could grow endlessly.
src/App.js
Outdated
| }, 1000); | ||
| }) | ||
| .catch(error => { | ||
| console.log(error); |
There was a problem hiding this comment.
Idea: Implement some error handling in the UI. console.log will break in IE.
src/App.js
Outdated
| newState[key] = list.filter( contact => contact._id !== id); | ||
|
|
||
| this.setState(newState); | ||
| return newState; |
There was a problem hiding this comment.
You could simplify this with ES6:
this.setState({
[key]: list.filter( contact => contact._id !== id)
});
| background-color: #99cfff; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
It would be cleaner to have the styles in the css files of the components.
…ng to redux but not using redux to get contacts from DB.
5 participants
Contact list