Uh oh!
There was an error while loading. Please reload this page.
Feature/timeline - #30
Conversation
lustoykov
left a comment
There was a problem hiding this comment.
I see a lot of manually creating html tags and manipulating them, usually we should do as much with react components as possible
| display: none; | ||
| } | ||
| &__itembox { |
There was a problem hiding this comment.
usually, it is a good convention to only nest modifiers (button--large would nest --large) but each element should get its own block (for button__icon __icon won't be nested). This leads to more consistent code that is easier to read. Not required to change anything just for info.
| MockDataService.fetchData(null).then((data) => { | ||
| this.setState({ data }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
also just for info, no need to change, you could make 1 liner out of that:
MockDataService.fetchData(null).then(data=>this.setState({ data }));Sorry if you know it already. Also I'd define the signature of fetchData like that: fetchData(entityId = null) with default param, so that you can call it like fetchData() (no need to pass null)
| Loading data... | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
also just for info: to avoid 2 returns, you could use either ternary:
return(<div>
this.state.data ? // render divs here...
: // render other divs here...
</div>)oreventhis.state.data&&(// render divs here)!this.state.data&&(// render other divs here)&&wouldimplicitlyconvertthis.state.datatoboolean(false/true),reactwouldthenevaluatethemandbasedonthemwilldisplaytheexpressioninthebracketsw/ooutputtingfalse,truehttps://facebook.github.io/react/docs/conditional-rendering.html#inline-if-with-logical--operator
| template: (event, element) => { | ||
| const className = 'timeline__itembox ' + | ||
| (event.icon ? 'timeline__itembox--hasicon ' : '') + | ||
| (event.linkType === 'external' ? 'timeline__itembox--external ' : ''); |
There was a problem hiding this comment.
if would be more readable if you abstract this in a function
constclassName=createClassName(event);| ); | ||
| return ReactDOM.render(html, element); | ||
| }, | ||
| zoomMin: 1000 * 60 * 60 * 24 * 3, |
There was a problem hiding this comment.
numbers seem a bit like magic to me, can we rename them meaningfully?
| <div> | ||
| <div className={ className } id={ 'timeline__itembox__' + event.id }> | ||
| { event.icon ? <div className="timeline__itembox__imagebox"><img src={ event.icon } /></div> : '' } | ||
| <div className="timeline__itembox__titlebox"> |
There was a problem hiding this comment.
consthtmlseems like a good candidate for a separate react component?
| /* Prepare the data */ | ||
| let idCounter = 0; | ||
| /* eslint-disable no-shadow */ | ||
| const { max, min } = events.reduce(({ max, min }, event) => { |
There was a problem hiding this comment.
I struggle to understand the purpose of this code snippet, can you abstract it in an own function with meaningful name?
| tooltipNode.innerHTML = tooltipContent; | ||
| tooltipNode.classList.add('timeline__tooltip'); | ||
| tooltipNode.id = 'timeline__tooltip__' + event.id; | ||
| container.appendChild(tooltipNode); |
There was a problem hiding this comment.
I think we could abstract this in a React component? Is that possible? And use in the render() function?
render(){return(<div>{events.map(event=><ToolTipevent={event}/>}</div>)}| const itemBoundaries = | ||
| document.getElementById('timeline__itembox__' + e.item).parentNode.parentNode.parentNode.getBoundingClientRect(); | ||
| const mouseX = e.pageX; | ||
| let left = mouseX; |
There was a problem hiding this comment.
getting a little bit difficult for me to track the logic, but consider doing this with react components, should be possible?
I'll stop here with the review, maybe we do one more iteration after you push the changes.
chaoran-chen
commented
Mar 19, 2017
After a phone call, we decided not to make any changes at the moment, and to introduce more abstractions (e.g. an own Tooltip component) then, when it is needed at some other places. |
kordianbruck
commented
Mar 19, 2017
@chaoran-chen thanks for adding that comment. I was just about to complain 🐤 |
Here is the first version of the timeline component. It uses the vis.js timeline library to display the actual timeline and contains some own code to create the tooltips.
Closes#17.
