-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ports - Amy W #3
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Very clean and functional solution -- I like that you didn't use any more lines of code than you needed and you obviously understand components and props. "You have met all learning goals for this project." 🎉 Well done!
</header> | ||
<main className="App-main"> | ||
<Timeline events={timelineData.events} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how neat and simple passing timelineData.events is.
|
||
const timelineComponents = props.events.map( (event, i) => { | ||
return ( | ||
<li key={i}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely works, and perhaps is even a better semantic solution, but since the TimelineEvent is a single thing, you could also just return that and not use a ul at all.
return ( | ||
<section> | ||
<ul> | ||
{ timelineComponents } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(See above comment -- again, not a fix, just an alternate idea)
}); | ||
|
||
return ( | ||
<section> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a CSS class provided called "timeline" that sets the width to 30%, margin to auto, and text-align left that you could throw in here.
{props.status} | ||
</p> | ||
<h5> | ||
<Timestamp time={props.timestamp}/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of the provided Timestamp component.
const Timeline = (props) => { | ||
|
||
const timelineComponents = props.events.map( (event, i) => { | ||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some provided CSS classes in TimelineEvent.css (timeline-event, timeline-event:hover, event-person, event-status, event-time) that would auto-style things for you :)
React Timeline
Congratulations! You're submitting your assignment!
Comprehension Questions