Code review comment for lp:~joergberroth/unav/20160328_fixes

Am 2016-03-28 um 18:04 schrieb Nekhelesh Ramananthan:
> Review: Needs Fixing
>
> Hi Joerg, thanks for the fixes. They're mostly good except for some minor issues that I have listed below,
>
Hi, thx, too.

> 1. The listitem text is not vertically centered in the settings page. See http://imgur.com/2ccNEsq. The tick padding is also incorrect.
Ah yes, I see.
I also think we shouldn't change the left padding of the text since the
rest of the core apps don't do that.
Well, I think there is no clear separation than. The former approach had
a visible separation. dekko does it via checkboxes.
The only other app I see with the new expandable than is clock.
Try this in clock:
-expand a setting list
-focus another app
- refocus the setting page
...you are totaly lost as you don't get the states at first sight.
I think this is not the best approach.

I did some testing and this looks good -> http://imgur.com/AzrnFAE
I could agree on that, too.
But, please re-think it on base of the expl. above.
Still, i like the indent more as it separates the list/action from the
"header".
>
> Here is the code diff, http://paste.ubuntu.com/15540311/
>
> 2. The delete icon on the list item is also weird. I think the text "Clear History" makes it clear what the listitem action does.

I think the icon clarifys that there is not only text but an action to take.
I had this discussion with marcos, too.
As it is 1:2 now, i will leave it out so far, again ;-) .
>
> An example from the core app "Clock", we have a settings option "Change Timezone"..the text alone does the job.
Can not find this action.... ;-)
> 3. I already agreed to the name change of HeaderListItem. Just one minor thing, the next time you move/rename a file, please do "bzr mv oldfile newfile". That is more cleaner. For this MP it is good.
ah yes, I'll do.

Thanks.