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

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

> > 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.
>
> Just updated the branch. Have a look. What do you think?

I think visually it is okay, but I'm not happy with the code. Please use padding.leading and padding.trailing to adjust the indentation instead of anchors. Refer to SlotsLayout documentation [1] Also while running your code, I saw warnings like Reference error: Unit is not defined and so on.

Also since you hid the subtitle and the divider when its expanded, how about we remove the indentation and instead change the font color of the options shown?

[1] https://developer.ubuntu.com/api/apps/qml/sdk-15.04.1/Ubuntu.Components.SlotsLayout/

>
> > 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.
>
> I fully see this!,
>
> >>> 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.
> but this action also (at least in the latest rc-prop version I am using)
> has an arrow as action indication ( and leads me to the system-setting page)
>

Yes the arrow is a standard design pattern used to indicate that it opens a new page. Had we instead shown a dialog, we would not have used any icon there. I spoke to the canonical designers about this.

« Back to merge proposal