Code review comment for lp:~nicolas-doffay/unity8/filter-selector

Revision history for this message
Nicolas d'Offay (nicolas-doffay) wrote :

Valid points, thanks for the in depth review! Regarding some of the topics:

===
211 + containerHeight: units.gu(38)
can you explain why 38? Seems unflexible to me. shouldn't this be relative to the total available height?

* At the request of design, they specifically requested it be 38 grid units and not relative.
===
370 + wait(100);

as noted before, this should be replaced by a tryCompare() to wait for the according things to happen

* This was why I used wait: https://pastebin.canonical.com/97075/

===

373 + compare(filterSelector.state, "expanded", "Filter selector did not expand.");
...
376 + compare(filterSelector.state, "collapsed", "Filter selector did not collapse");

This test is not really good. While it tests if internally the state really changes, it doesn't really test that the user actually can see that. It would be better to grab the Rectangle for the overlay, and tryCompare() that it eventually reaches the size which you have set in scopeViewHeight (or filterSelectorHeight once the above comment is fixed). Also, tryCompare() that the selectors actually become visible and hide again when collapsing.

* Fair enough, but as mentioned before setting custom models on this specialised delegate requires a lot more variables to be exposed as aliases (including in the SDK) which I feel should be kept private. This is the reason I'm loathe to write more tests for the individual selectors.

« Back to merge proposal