Conversation
src/components/DataTable.js
Outdated
| } | ||
|
|
||
| componentWillMount() { | ||
| this.setState({ |
There was a problem hiding this comment.
why do we have to setState again, when we already have a default state? because we don't get headers until now?
There was a problem hiding this comment.
Yeah, exactly. I tried to put it in the constructor, but this.props.headers wasn't defined yet.
There was a problem hiding this comment.
Would props.headers in the constructor work? this isn't defined at that point, but props does get passed in as an argument.
There was a problem hiding this comment.
Oh, good call! Yeah, that worked. I'll update.
|
looks awesome generally! just put some comments in line, but i would also merge as is. great work. |
|
Thanks for the feedback, @samuelcole! The one issue I ran into with this is that the pagination makes it so that the columns jump in size when you sort (because there might be a longer column value on pg 2 that suddenly gets bumped to pg 1 on sort). I'm not totally sure how to fix it and am not sure whether having sorting functionality outweighs the weirdness of the column jumpiness, so see what you think and let me know! |
| values = values.slice(start, end); | ||
| const columns = props.headers; | ||
| const standardColumnWidth = 100; | ||
| const wide = (columns.length * standardColumnWidth < window.innerWidth); |
There was a problem hiding this comment.
We have wide in the initial state, but it doesn't seem to be used anywhere -- should this calc (and standardColumnWidth) be moved to the constructor?
3 participants
For #11.
TODO: