Converted Body, TBody, Tbody/Row and Tbody/Row/Cell to glimmer #351
Conversation
There was a problem hiding this comment.
Thanks a lot for that work :) I agree with you on changing component by component.
Just before you submit #344 I've asked @miguelcobain if glimmer migration was planned. I missed your issues/PRs 😓
I would gladly help if necessary, I will be available today, tomorrow, next monday (02/08), and after 02/15.
|
|
||
| registerCell(cell) { | ||
| let columns = this.get('columns'); | ||
| let prop = cell.get('prop'); | ||
| let columns = get(this, 'args.columns'); |
There was a problem hiding this comment.
do we need Ember.get() here ?
There was a problem hiding this comment.
No we dont, but I was keeping as I didnt know if 2.18+ was being supported. After looking at the polyfills 3.4 is the farthest we can go, so I will be changing the gets that can be changed
| @@ -1,4 +1,4 @@ | |||
| {{#if this.visible}} | |||
| {{#if this.column.visible}} | |||
There was a problem hiding this comment.
So it seems like a change here, as the cell itself can't have its own visible flag anymore.
This is probably fine (I don't see why a column would be visible, but some of the cells wouldn't), but that's a breaking change to me.
There was a problem hiding this comment.
Technically the visible flag is passed from the column, although the user could override it, it leads to problematic HTML, which is why I didnt write it in such a way as to provide the ability to override it. I dont feel it should be documented as a user defined field for just this reason. Just because it was available, doesnt mean you should :)
I believe this is going to be a major bump anyway.
|
Closing this to get a couple of PRs first, will then rebase and submit again |
Hope its ok to do small manageable chunks. Not a fan of changing all components at one time, plus easier for you to review in smaller pieces
#344