Fix data table height not calculating correctly

[tomnez]

Overview

Cell heights weren't being recalculated when underlying data changed (#161)

The recalculation issue is fixed in Polaris by recalculating height when the DataTable component updates.

On hold

This fix doesn't appear in the React version until v3.0.0-beta.11, so we should hold off on merging until we have a 3.0 version of ember-polaris just to keep our versions in sync. (we'll ship now and add a comment)

Demo

[andrewpye]

This isn't in version v2.11.0 so I'm not sure if we should repoint this to master or not include it at all yet until we move up to a later version of Polaris where they include it too

My gut says wait until it lands in React world so we're parallel with them as far as possible. We can leave this branch here and if anywhere needs the fix, they can just reference this branch.

The React version gets away with using childNodes because they use row.childNodes as NodeListOf

Good catch, I never stopped to think about why childNodes worked for the React version but we had to use children 👍

[andrewpye]

I wonder if we're better off using didRender rather than didUpdateAttrs - reason being that we could potentially be passing objects to represent content (e.g. a hash of componentName and props); if the content changes but the object reference stays the same, I think didUpdateAttrs won't trigger, but didRender will.

[tomnez]

Ohh good call. I'll update 👍

[tomnez]

@andrewpye updated to use didRender and also took out the childNodes fix which I will open in a separate PR.

[andrewpye]

Changes look good @tomnez - if this isn't to be merged until a future Polaris update, can you mark it as such please so we know when this can be merged & shipped? 👍

[vladucu]

since this is a bug and it's just an implementation detail, not involving a changed API or anything...what do you guys think of just shipping this now?

NOTE: I don't think we should be 1:1 with any issues in the React implementation 😜

[andrewpye]

I don't think we should be 1:1 with any issues in the React implementation

I do see your point, but at the same time, fixing bugs in parallel with the React implementation does make issue tracking/resolution more straightforward in Ember-land...

[vladucu]

I do see your point, but at the same time, fixing bugs in parallel with the React implementation does make issue tracking/resolution more straightforward in Ember-land...

we should then maybe add a comment, but definitely not wait to update to matching react version if it was already fixed in React in a newer version...especially if it's a straight-forward fix

[tomnez]

Ok, we'll get this in now after the build completes instead of waiting. I'll add a comment 👍

[vladucu]

thx boizz
🚀

[tomnez]

@andrewpye reverted back to using didUpdateAttrs, not sure what's going on in ember test land, but using didRender seemed to timeout all the tests for polaris-data-table 🤔I guess can revisit if didUpdateAttrs turns out to be not good enough.