Code review comment for lp:~unity-team/unity8/unity8.previews_textSummary

On 04.02.2014 18:56, Andrea Cimitan wrote:
>> =====
>>
>> 231 + property bool more: false
>>
>> I wonder, shouldn't this be read only?
>>
>
> Thought about the case someone wants to start the widget with the text already expanded, so setting this to true

Makes sense.

>> =====
>>
>> 321 + implicitHeight: titleLabel.visible ? titleLabel.height +
>> textLabel.height : textLabel.height
>>
>> childrenRect.height no good?
>>
>
> Visibility doesn't affect childrenRect

Visibility doesn't, but you're changing anchors based on visibility, and
that does affect childrenRect.

>> =====
>>
>> 329 + top: parent.top
>>
>> Not needed, is it?
>>
>
> I always specify all dimensions, or three anchors and relative remaining dimension

Not sure that helps readability a lot, but definitely costs us more
memory and CPU cycles.

>> =====
>>
>> 514 + var mappedTextLabel = root.mapFromItem(textLabel, 0, 0)
>> 515 + compare(mappedTextLabel.y, titleLabel.height)
>>
>> They're in the same geometry, do we really need the mapFromItem?
>>
>
> We're testing the placement now, and then when it disappears and moves on top. Comment added

> 515 + // verify titleLabel is visible and textLabel is anchored below it

Why isn't that simply textLabel.y == titleLabel.height?

http://paste.ubuntu.com/6874503/