-
Notifications
You must be signed in to change notification settings - Fork 38
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
Added timeline and data to timeline, and created timeline.test.js file #9
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.
Code looks clean and easy to understand
@@ -0,0 +1,7 @@ | |||
import React from 'react'; |
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.
What else can we test about this component? Think about the different states that it can be in and how you might test that it is or isn't in one of those states.
import { Timeline, TimelineItem } from 'vertical-timeline-component-for-react'; | ||
import Moment from 'react-moment'; |
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.
Was moment removed in this pull request?
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.
Confirm, Moment was removed.
<Timeline lineColor={'#</Timeline>ddd'}> | ||
{timelineQuery.data.map(details => { | ||
return ( | ||
<TimelineItem |
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 think we should consider moving this TimeLineItem component to its own file. It has a few moving pieces and I think digesting it on its own will be simpler for an unfamiliar dev. It also increases the maintainability of the component because now someone can work on changes to the list item while someone does other changes to the list itself.
Non-critical
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.
So we can save this for the second release? So the PR can be merged after the testing is finished?
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.
Code looks clean and working
Description
Created Timeline, and populated it with the 5 most recent timeline events. Added nanoid library to give unique keys to items, and created a function to replace the 'source' link with names to the actual websites (and made websites open in new tab). Also added CSS to center header and display timeline event details according to mock-up. Replaced Moment with luxon to parse dates.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Change Status
How Has This Been Tested?
Test A Timeline.test.js was created to test component rendering. Will add more tests once the modal is added.
Test B
Checklist
Note: this is one error message that is appearing in console that is being caused by an internal issue with the Timeline library. This is a known issue and is currently documented here joshwnj/react-visibility-sensor#178.
![findDomNode](https://user-images.githubusercontent.com/14119349/102046584-36010e80-3da1-11eb-923d-1599173a3c13.png)
This error does not affect the rendering of this component, nor will it affect any other components or cause any breaking changes. This error should not appear in production.