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

Revision history for this message
Nekhelesh Ramananthan (nik90) wrote :

> 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".

Alright go with the indentation. But please use padding.leading: units.gu(1) to move the text. Otherwise it breaks the listitemlayout visually. In this case, you might also then want to indent the tick icon. SO add padding.trailing: units.gu(1). Please test this.

> > 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./me
>
> 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 ;-) .

Now that uNav is a core app shipped by default on the phone, it is even more important that it follows the design standards of other core apps like Dekko, Clock, Weather, Music app etc. None of those apps display such an icon in a list item. We want to maintain consistency. That's what I am trying to get at.

> >
> > 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.... ;-)

Its in the clock app settings page right at the bottom.

« Back to merge proposal