917339 - [Clock] Timer Sound should use same input type as Alarm Sound

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Clock, defect)

Description

[Corey Frang [:gnarf]]

The timer sound selection currently uses a standard select box, it should be using the same select box as the alarm sound.

Comment 1

[Brendan McLoughlin [:bmac]]

Attachment: Pull request on github

Comment 2

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807409 [details] [review]
Pull request on github

Hey Brendan,

Since you, Corey and I talked about this in person, I'm going to un-set the review flag for now. Please re-set it whenever you're ready for me to take another look!

Comment 3

[Brendan McLoughlin [:bmac]]

Attachment: Pull request on github

Comment 4

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807957 [details] [review]
Pull request on github

Hey Brendan!

I've left my feedback on the GitHub pull request.

For future reference: when you are requesting a new review of an existing pull request, you can re-use the attachment you already created for the bug on BugZilla. Just go to its "Details" page and flag your reviewer again. (The ability to upload additional attachments is more useful when you're attaching patch files directly, and the old attachment is no longer valid. When we use GitHub pull requests, the attachment object doesn't really get outdated in this way.)

Comment 5

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807957 [details] [review]
Pull request on github

Almost there, Brendan! Just a couple minor details remain.

Comment 6

[Brendan McLoughlin [:bmac]]

As per a discussion with Mike Pennisi I have updated the pull request to only contain the simplest possible fix for this issue. I will open a second bug and pull request that will include the refactoring changes I have made.

Comment 7

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807957 [details] [review]
Pull request on github

Thanks for this, Brendan! One minor detail and we should be good to go: the "FormButton" (to borrow a term from your now-deferred refactoring) in the Timer panel should have a slightly different presentation than the "FormButtons" found on the "Add/Edit Alarm" panel:

https://mozilla.app.box.com/applications/1/1092493904/9862011878/1

Comment 8

[Brendan McLoughlin [:bmac]]

Thanks Mike. I have updated the pr to fix the visual differences between my implementation and the spec.

Comment 9

[Mike Pennisi [:jugglinmike]]

Comment on attachment 807957 [details] [review]
Pull request on github

Looking good! I'll merge this asap

Comment 10

[Mike Pennisi [:jugglinmike]]

master: https://github.com/mozilla-b2g/gaia/commit/90758c0f1e697dd7de8c23f8311c3d3f476c85d9

Thanks, Brendan!

Comment 11

[Mike Pennisi [:jugglinmike]]

Attachment: 917339-compare.png

Comparison of UI before and after this patch is applied.

Comment 12

[Mike Pennisi [:jugglinmike]]

Requesting "koi+" because the current unstyled select input is way off specification. See comment 11 for a visual comparison.

Comment 13

[Dylan Oliver [:doliver]]

triage: koi+ for spec/style consistency

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 90758c0f1e697dd7de8c23f8311c3d3f476c85d9 to:
v1.2: 6ff85a712bff00aa2663639cd3ea15226aeb8b13

Comment 15

[Shilpa Suresh]

This issue no longer occurs on Buri v 1.3 master and v 1.2 aurora 10/16 build
The timer sound selection box uses the same select box as the alarm sound.

10/16 v 1.2 aurora

Environmental Variables
Device: Buri v 1.2.0 Mozilla RIL
Build ID: 20131016004005
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/75b0b968f3ed
Gaia: 5ef3535021286ccab7af639897feaaf5955720a0
Platform Version: 26.0a2
Firmware Version: US_20130912_QC211

10/16 v 1.3 master 

Environmental Variables
Device: Buri v 1.3.0 Mozilla RIL
Build ID: 20131016040202
Gecko: http://hg.mozilla.org/mozilla-central/rev/9f63bbc00527
Gaia: 3d4f1107e6e91e5f5649edc0f2565ac837111d7d
Platform Version: 27.0a1
Firmware Version: US_20130912_QC211