Christopher.Perkins.Austin1#8
Conversation
|
last commit msg should read 'implemented axios POST and DELETE request...' |
ivome
left a comment
ivome
left a comment
There was a problem hiding this comment.
Great job, really clean code, easy to understand and nice structure (reusable functions etc.)! Keep up the good work!
| <img src={props.avatar} alt="avatar" /> | ||
| </div> | ||
| <div className="contact-info"> | ||
| <ContactName |
There was a problem hiding this comment.
Everything looks really good, but when I ran your project I noticed I could only select by clicking on the contact name. From a UX standpoint I think it could be better. First, whichever element you choose to attach the event listener to, I think you should use cursor: pointer to make it obvious to the user that they can click. Second, I recommend using the entire <li> element for the event listener. Then it's just a matter of adding `event.stopPropagation()' to any methods that are going to be passed to click listeners attached to children (like the remove button).
There was a problem hiding this comment.
Great ideas, thank you! Yeah I had tried to set them as <a> elements but didn't understand how to pass the event objects through the chain to preventDefault(). I think I more-or-less understand how to pass the event object now, so I am going to make that change to an <li> with a pointer on hover. I also hadn't thought about stopPropagation(), great ideas! I didn't really know what stopPropagation() did besides being similar to preventDefault(), this article helped me understand it, then I remembered you saying it prevents the event bubbling which is pretty much exactly what it does. Good stuff, thanks for taking the time to test it out and give such great feedback!
alexjgaw
left a comment
alexjgaw
left a comment
There was a problem hiding this comment.
Everything looks really good, you just need to finish the extensions when you can. Whatdyou have a new job or something?
| if (props.name.toLowerCase().indexOf(stringToHighlight) >= 0) { | ||
| const highlightedContactBeginning = contactName.slice(0, searchTermStartIndex); | ||
| const highlightedContactEnd = contactName.slice(searchTermEndIndex); | ||
| const highlightedContactMiddle = contactName.slice(searchTermStartIndex, searchTermEndIndex); |
There was a problem hiding this comment.
What about multiple matches? Example: Sp in "Spike Spiegel" 😄
| background-color: #B9F6CA; | ||
| border-color: #fff; | ||
| padding: 2% 2%; | ||
| } |
There was a problem hiding this comment.
CSS should ideally be placed in the corresponding css file of the react component.
3 participants
No description provided.