[GridList] Remove style-propable mixin #3202
Conversation
| }, | ||
|
|
||
| componeneDidUpdate() { | ||
| componentDidUpdate() { |
There was a problem hiding this comment.
I noticed the componentDidUpdate method was spelled wrong as componeneDidUpdate. Unless React's lifecycle methods support spelling checking, I'm guessing this wasn't doing anything 😆
I named it back, which obviously is changing the behavior. Though I'm thinking that the behavior is probably right to do that stuff on both mount and update.
There was a problem hiding this comment.
I guess you're implicitly fixing a reported issue. I don't remember it... but I'm sure this was affecting the behavior in a bad way. O.o
|
All good 👍 |
| @@ -105,16 +102,16 @@ const GridList = React.createClass({ | |||
| } | |||
| const childCols = currentChild.props.cols || 1; | |||
| const childRows = currentChild.props.rows || 1; | |||
| const itemStyle = this.mergeStyles(styles.item, { | |||
| const itemStyle = Object.assign({}, styles.item, { | |||
There was a problem hiding this comment.
I'm wondering, why using Object.assign({}, styles. in some cases and Object.assign(, styles. in others?
There was a problem hiding this comment.
Good point, it wasn't necessary here. My faulty logic when I did this was because it's inside a loop of children and I didn't want styles.item to be mutated and contain data from other items so I decided to create an new object each time. But in this case the keys never change so they should always be overwritten so it should be safe.
There was a problem hiding this comment.
I haven't noticed the loop 👍.
I would say let's keep our migration safe. We can tweak and improve stuff later.
[GridList] Remove style-propable mixin
|
@newoga Thanks! |
zannager
added
the
package: styles
Noticed something in the code that I fixed, I leave a comment on that line.