ciena-frost / ember-frost-table

MIT License
1 stars 9 forks source link

Fix column alignment issue when new rows were added after first render #36

Closed AdamWard1995 closed 7 years ago

AdamWard1995 commented 7 years ago

This project uses semver, please check the scope of this pr:

CHANGELOG

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling bc5dca9412246066b78c4c2aae1ac22f60bd07d2 on AdamWard1995:flexTable into on ciena-frost:master.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 3756eb0e9d4a3cd18ce932570637b54a82e1735d on AdamWard1995:flexTable into on ciena-frost:master.

notmessenger commented 7 years ago

The didRender hook is called during both render and re-render after the template has rendered and the DOM updated.

whereas the didInsertElement hook is called only during initial render. I notice in this PR a lot of refactoring from using didInsertElement to using didRender but a lot of the code involves the settings of minimum widths, etc. Do these values need to be recalculated on rerender or should they be left to be set in didInsertElement?

AdamWard1995 commented 7 years ago

@notmessenger I had originally wrote it as using didInsertElement so that the cells would only be aligned once. As I tried to integrate this in mcp-ui I noticed that as rows are added and removed through filtering for example, the new rows didn't get aligned

notmessenger commented 7 years ago

@AdamWard1995 are all (re)calculations needed or just some of them? If all that's fine, just seems while glancing at it (only) that minimum widths would not need to be, for example.

AdamWard1995 commented 7 years ago

So in the case that a new row is added, a cell may be wider than the current minimum width in which case the whole column needs to be realigned. I can play for a bit and see if it is possible to only do the alignment when this situation occurs

AdamWard1995 commented 7 years ago

@notmessenger did a best effort to minimize the number of DOM accesses. It should only realign the entire table when the number of rows changes

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling e59d04ce3d336130a6868fea377e5a7f293c1cb8 on AdamWard1995:flexTable into on ciena-frost:master.