Unity 8

Code review comment for lp:~unity-api-team/unity8/modeminfo

modeminfo
Merge into trunk

Revision history for this message

Antti Kaijanmäki (kaijanmaki) wrote on 2014-07-02:

> MP checklist:
> https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8
>
> please add required branch from indicator-network.

Part of the MP description.

> Item visual design doesn't match:
> https://docs.google.com/a/canonical.com/document/d/1OyHUg_uUfmhDNa-
> 9UrMc1tZ_eH_99PEU_V2l1YFA1UY/edit#
> Don't know which one needs updating.

That Google document is not the working visual design for indicator-network. The tentative one is here https://wiki.ubuntu.com/Networking, but does not cover these use cases.

The best we can do ATM is to try to do sensible choices between the two designs.

> 1) Status icons on left (no roaming icon on right, perhaps should go left).
> - should probably use an "x-canonical-modem-status-icons" action which has
> a "a(s)" variant rather than an action per icon.

Roaming icon needs an explanatory text to go with it, so just having a list of icons is not going to cut it.

> 2) Text should be right aligned and be in format (CARRIER - NUMBER).

The gdoc visual design is not complete and should not be considered obligatory.

> 3) There apparently shouldn't be SIM unlocking in indicators.

indicator-network is the only place where the user can manually unlock the SIM.

> 4) Is the text under roaming icon needed?

Yes, as the roaming icon is not clear enough on it's own.

> 66 +
> shell.hideIndicatorMenu(UbuntuAnimation.BriskDuration);
>
> While this is in other items at the moment, we're no longer going to be hiding
> the indicators on activations. Please remove.

When was this decided? How do the indicators close then if you select "Wi-Fi settings" or any other item inside the indicators that open applications?

> 45 + property var simIdentifierLabelAction:
> QMenuModel.UnityMenuAction {
>
> Why is this text an action? Will it ever change?
> Should this not be the label of the menu item. actions. I realise this is a
> backend related comment.

Nothing specifies that the sim identifier could not be changed. Having it changeable gives flexibility on the backend side.

> MP checklist:
> https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8
> 
> please add required branch from indicator-network.

Part of the MP description.

> Item visual design doesn't match:
> https://docs.google.com/a/canonical.com/document/d/1OyHUg_uUfmhDNa-
> 9UrMc1tZ_eH_99PEU_V2l1YFA1UY/edit#
> Don't know which one needs updating.

That Google document is not the working visual design for indicator-network. The tentative one is here https://wiki.ubuntu.com/Networking, but does not cover these use cases.

The best we can do ATM is to try to do sensible choices between the two designs.

> 1) Status icons on left (no roaming icon on right, perhaps should go left).
>    - should probably use an "x-canonical-modem-status-icons" action which has
> a "a(s)" variant rather than an action per icon.

Roaming icon needs an explanatory text to go with it, so just having a list of icons is not going to cut it.

> 2) Text should be right aligned and be in format (CARRIER - NUMBER).

The gdoc visual design is not complete and should not be considered obligatory.

> 3) There apparently shouldn't be SIM unlocking in indicators.

indicator-network is the only place where the user can manually unlock the SIM.

> 4) Is the text under roaming icon needed?

Yes, as the roaming icon is not clear enough on it's own.

> 66      +
> shell.hideIndicatorMenu(UbuntuAnimation.BriskDuration);
> 
> While this is in other items at the moment, we're no longer going to be hiding
> the indicators on activations. Please remove.

When was this decided? How do the indicators close then if you select "Wi-Fi settings" or any other item inside the indicators that open applications?

> 45      +            property var simIdentifierLabelAction:
> QMenuModel.UnityMenuAction {
> 
> Why is this text an action? Will it ever change?
> Should this not be the label of the menu item. actions. I realise this is a
> backend related comment.

Nothing specifies that the sim identifier could not be changed. Having it changeable gives flexibility on the backend side.

« Back to merge proposal