Code review comment for lp:~nik90/ubuntu-clock-app/edit-alarm-feature

Revision history for this message
Victor Thompson (vthompson) wrote :

Just a note that browsing the ringtones does not work on Trusty, conceivably because /usr/share/sounds/ubuntu/ringtones does not exist. This defaults to the current directory in this case. Perhaps you need some sort of fall back to show nothing when this directory does not exist. Otherwise the user can select random files in the current directory. Likewise, filtering on ogg and mp3 files might be a good idea.

Consider making the naming on the AlarmLabel.qml consistent. The page title is "Label", but the Label component on top of the text field is "Alarm Label". Consider getting rid of the Label "Alarm Label" since it is redundant information. Similarly, the word "Label" is poor terminology. I know it is what is in the design spec, but it is a poor term for a title or name of an alarm.

In my opinion there should not be a default "label" for a new alarm. The user should simply be required to add a label. No real person makes an alarm and lets it be labeled "Alarm #1".

The new alarm date picker should conform to the user's locale. In my locale I should see 7 AM, 12 PM, etc for 12 hour time.

When selecting multiple dates for an alarm, say Monday and Tuesday, when they are displayed in a comma separated fashion there is no space between them. Instead the days are displayed as "Monday,Tuesday". It'd be best to have a space in between.

There are numerous instances where "anchors" properties can be grouped. Consider doing so.

review: Needs Fixing

« Back to merge proposal