Code review comment for lp://qastaging/~nik90/ubuntu-clock-app/replace-alarmsound-checkbox

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

> Comments:
>
> 1. Please update the pot file for the new strings.

Will do once we agree on the wording of the headers to be used.

> 2. Could we indent the alarm sound delegate text? This might help to make the
> section headings more obvious? This is just an opinion, however.

I agree that the section headings need to be more obvious and I am a bit conflicted on how to proceed. If I indent the alarm sound delegate text, it breaks consistency with section headers shown by the address-book-app and the world city page in the clock app. Right now, the page looks very busy with so many listitem dividers. I will try out a couple different variations and see which one maintains consistency, clean loook etc.

> 3. As part of this MP could you make it so the when the user selects the
> original alarm sound in the AlarmSound page that the saveAction is still
> enabled? It's weird if you play a few and get back to the original and you
> can't save it. IMO it should always be enabled.

Hmm I can see how it gets annoying after you makes changes and then revert back to the original sound. At the moment, the save button is enabled *only* when you change the value to something new. We follow this pattern everywhere now. I suppose I can change that to enabling the save button if *any* change is done regardless of whether that changes defaults to the old/new value. I will have to do this in 3 different places and also fix the unit tests. A separate MP is a better idea.

> 4. I wonder if it'd be useful to put a small play/pause toggle next to the
> tick box. This would make it more apparent that tapping the item again will
> stop the alarm playback.
>

Hmm good idea. I will add this.

« Back to merge proposal