James Kuhr Advanced 1 by jamesku · Pull Request #6 · AustinCodingAcademy/advanced-contact-list-starter

jamesku · 2017-01-24T02:42:39Z

No description provided.

michaeb · 2017-02-04T03:10:14Z

src/App.js

+
+  handleSelect(value) {
+    let result = this.state.data.contacts.filter( function (obj) {
+      return obj._id == value;


Careful using the double == operator, it does some strange parsing that can lead some hard to track down errors. In general I think it's better to use the triple equals just for simplicity.

Here's a little blurb:

"double equals will perform a type conversion when comparing >two things; triple equals will do the same comparison without type >conversion (by simply always returning false if the types differ);

Great comment. I would follow the rule to NEVER use the == operator. I thought I added the eslint rule for that...

michaeb · 2017-02-04T03:17:26Z

src/components/contact.js

+
+    this.name = this.props.name.toLowerCase();
+    this.searchValue = this.props.searchValue.toLowerCase().trim();
+    this.pos = this.name.search(this.searchValue);


Nice solution for the highlighter!

michaeb · 2017-02-04T03:18:04Z

src/components/text.js

@@ -0,0 +1,12 @@
+import React from 'react';


Good thinking breaking out the text into it's own component

michaeb · 2017-02-04T03:34:31Z

src/components/button.js

+}
+
+Button.propTypes = {
+  resetSelection: React.PropTypes.isfunc,


Close! I made a similar error. For a function proptype, the syntax is React.PropTypes.func , generally you'd also want to make it required which looks like this: React.PropTypes.func.isRequired

michaeb · 2017-02-04T03:35:07Z

src/components/contact-list.js

+      <ul className="contact-list">
+        {contacts.map((contact,i) => {
+          return (
+            <Contact {...contact} handleSelect={this.props.handleSelect} key={i} searchValue={this.props.searchValue} />


Nice use of the spread operator, I'm gonna refactor my code to do this.

ivome

Great job on the implementation! There are a few things in your code that eslint should tell you about.

ivome · 2017-02-06T03:04:38Z

src/App.js

+import Button from './components/button.js';
+import axios from 'axios';
+
+const data = {


I wouldn't recommend using local variables and mix it with state. This will create a whole bunch of problems when you have multiple instances of the same component on your page. They use the same data and the values get mutated in different locations. Better exclusively keep that in the component state.

ivome · 2017-02-06T03:05:45Z

src/App.js

+  componentDidMount() {
+    axios.get('http://localhost:4000/contacts')
+      .then(resp => {
+        data.contacts = resp.data;


Mutating the data object is dangerous, since in your implementation all instances of the Component share the same variable. Also see comment above. Just use state for that and remove the local variable above.

ivome · 2017-02-06T03:06:08Z

src/App.js

+        this.state.data.contacts.map((obj) => {
+          obj.isSelected = false;
+        })
+        );


Indentation.

ivome · 2017-02-06T03:10:39Z

src/App.js

+    this.setState(
+        this.state.data.contacts.map((obj) => {
+          obj.isSelected = false;
+        })


setState always takes an Object as an argument. You are passing an array (the return value of the map function). This only works, because you are mutating the state directly through obj.isSelected. Mutation of the state should not be used at all in react. If you want to change something, you have to clone the objects.
You are making this harder than it is because you are storing everything in a data object inside of state. Better keep the state object flat and store everything on it directly.

ivome · 2017-02-06T03:11:48Z

src/App.js

+
+  handleSelect(value) {
+    let result = this.state.data.contacts.filter( function (obj) {
+      return obj._id == value;


Great comment. I would follow the rule to NEVER use the == operator. I thought I added the eslint rule for that...

ivome · 2017-02-06T03:13:59Z

src/components/contact-list.js

+  data: React.PropTypes.array,
+  name: React.PropTypes.string,
+  occupation: React.PropTypes.string,
+  handleSelect: React.PropTypes.isfunc


Proptype should be PropTypes.func, as mentioned above. Name for functions are usually called onSomeAction.

ivome · 2017-02-06T03:14:45Z

src/components/contact.js

+import React from 'react';
+import Text from './text.js';
+
+const divStyle = {


Always try to use descriptive variable names

ivome · 2017-02-06T03:15:01Z

src/components/contact.js

+            <Text divStyle={divStyle} text={this.text2()} />
+            <Text divStyle={divStyle2} text={this.text3()} />
+          </span>
+                </h2>


Indentation

ivome · 2017-02-06T03:15:47Z

src/components/search-bar.js

@@ -0,0 +1,20 @@
+import React from 'react';


File names should have the same name as the component name.
This could also be turned into a functional component.

ivome · 2017-02-06T03:16:32Z

src/components/text.js

@@ -0,0 +1,12 @@
+import React from 'react';
+
+export default class Text extends React.Component {


Use functional components for only rendering things.

…and POST requests.

ivome

Great job! Keep up the good work!

ivome · 2017-02-11T16:39:50Z

src/App.js

+    this.setState({
+      selected: newSelection
+    }
+    );


Indentation... :-)

ivome · 2017-02-11T16:40:40Z

src/components/ContactForm.js

+    this.setState({
+      picture: event.target.value
+    });
+  }


Did you see the extension in the curriculum for how to write the handler logic in one function?

ivome · 2017-02-11T16:41:28Z

src/App.js

+        this.state.contacts.map((obj) => {
          obj.isSelected = false;
        })
        );


Indentation...

jamesku added 4 commits January 23, 2017 20:41

basic contact list, search not working

6577d95

App with all functionality (including extra stuff)

ec0c54d

fixed one spacing issue with contacts

496f8e0

data now retrieved via Axios

353ed8b

michaeb reviewed Feb 4, 2017

View reviewed changes

src/components/text.js

@@ -0,0 +1,12 @@

import React from 'react';

Copy link

michaeb Feb 4, 2017

Choose a reason for hiding this comment

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

Good thinking breaking out the text into it's own component

michaeb reviewed Feb 4, 2017

View reviewed changes

ivome reviewed Feb 6, 2017

View reviewed changes

Contact list refactored to address comments to date and includes GET …

57b497b

…and POST requests.

ivome reviewed Feb 11, 2017

View reviewed changes

		@@ -0,0 +1,12 @@
		import React from 'react';

		export default class Text extends React.Component {

Conversation

jamesku commented Jan 24, 2017

Uh oh!

michaeb Feb 4, 2017 • 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!

ivome 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!

ivome 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaeb Feb 4, 2017 •

edited

Loading