Skip to content

Week #2 homework#17

Open
cirept wants to merge 5 commits into
modern-web-application-uw18:masterfrom
cirept:master
Open

Week #2 homework#17
cirept wants to merge 5 commits into
modern-web-application-uw18:masterfrom
cirept:master

Conversation

@cirept

@cirept cirept commented May 7, 2018

Copy link
Copy Markdown

Week 2 HW Submission

Please fill out the information below in order to complete your assignment. Feel free to update this comment later if necessary.

  • Comfort rating on this assignment (1-5):
    3
  • Completion rating on this assignment (complete/incomplete):
    complete
  • Github handles of code I've reviewed:
    n/a

cirept and others added 5 commits May 4, 2018 00:19
Finished styles, cards, and extra credit.
added smoke tests and default prop values
@cirept cirept closed this May 11, 2018
@cirept cirept reopened this May 11, 2018
Comment thread social-network/src/App.js
* @param {object} item - The article object in the array
*/
const listItems = YourArticles.map((item, ind) => {
const { link, image, description, title, author, postedDate, minutesToRead } = item;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't destructure the properties here, but rather pass the whole {item} as a prop to Card component. This way the component is responsible for its own internals

@@ -0,0 +1,28 @@
import React,{Component} from 'react';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure whether there's enough functionality and re-usability there to justify a separate component for Summary and Image

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I kind of went down the rabbit hole on this one. I just kept breaking things down until I couldn't anymore. >.< Lesson learned. =]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants