918400 - [Clock] Vibrate should use a toggle instead of a drop down

Status: VERIFIED FIXED

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

Description

[Eric Pang [:epang] UX]

Hi Gareth,

Hoping you can help out with this small change.  When setting a new alarm in the clock app the Vibrate button currently uses a drop down with the options "On and Off".  For an action like this we should be using a toggle switch instead.  Can you help make this update?  Feel free to reassign if needed, Thanks!

Comment 1

[[e-mail], [e-mail]]

Hi Eric,

Gareth might works on marionette test and Email app. Not sure he is available or not. I'm okay for user experience change. Would UX team have the visual for the toggle switch reference? Thanks.

Comment 2

[Eric Pang [:epang] UX]

Attachment: Edit Alarm (with vibrate).png

Hi Ian, here's a mock up with the toggle being used for vibrate.  Thanks!

Comment 3

[Eric Pang [:epang] UX]

(In reply to Eric Pang [:epang] from comment #2)
> Created attachment 808519 [details]
> Edit Alarm (with vibrate).png
> 
> Hi Ian, here's a mock up with the toggle being used for vibrate.  Thanks!

Hi Ian, the toggle used will be the one from the building blocks.  Is anything else needed to help this bug move forward?  Let me know, thanks!

Comment 4

[Eric Pang [:epang] UX]

Closing based on new clock refresh bugs opened for 1.4

Comment 5

[Eric Pang [:epang] UX]

Sorry, closed the incorrect bug.

Comment 6

[Vishnu Dangi]

as there is no update kindly assign to me.

Comment 7

[Eric Pang [:epang] UX]

Thanks Vishnu!  Pavel when ready I can review the visual can you help review the tech?  Thanks!
Eric

Comment 8

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Sure,
we will need final approve before land this but I can take a look before that

Vishnu: feel free to work on it :)

Comment 9

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Vishnu,
any updates on this one?

Comment 10

[Vishnu Dangi]

some problem arises in java script.i will try resolved them.

Comment 11

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

OK Thanks :) just keep me up to date and if you have problems ping me on IRC or here with link to your PR/patch

P.S. You don't need to use "Need more information from" input if you don't need it :)

Thanks for the help!

Comment 12

[Marcus Cavanaugh [:mcav]]

Hey Vishnu, how's this one going? Not trying to rush you, I just want to make sure that bugs that are assigned are still being worked on. :-)

Comment 13

[Vishnu Dangi]

i have some problem in java script related.i working it.

Comment 14

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Vishnu,
you can attach a link to PR and we can help you with some suggestions and after that you can `rabase` and update the PR and ask for r+

Comment 15

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: patch for Gaia/master

Hey Mike,
I made a quick patch for this one ... can you take a look? Thanks in advance

Comment 16

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8370736 [details] [review]
patch for Gaia/master

Hi Pavel,

I've left some feedback on your pull request on GitHub. Note that the patch is currently failing unit tests and Gaia UI tests. I'm not familiar with the UI tests, but you may have to make some changes there (in addition to the ones I've requested) in order to get them to pass.

Comment 17

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8370736 [details] [review]
patch for Gaia/master

Hey Mike,

Thanks for the feedback, was very helpful :) I just have one question.
before my patch and after it I cant really stop the vibration ... with my patch I think that I set correctly the this.alarm.vibrate option but it doesn't work. Is there a bug for this or the problem is somewhere in my patch?

Comment 18

[Marcus Cavanaugh [:mcav]]

Pavel, that's bug 971162, I'm looking at that now.

Comment 19

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Marcus,

thanks for the quick answer ... so the problem is not on my side ... I'm cool now

Comment 20

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8370736 [details] [review]
patch for Gaia/master

Hi Pavel,

I've left some more feedback on your pull request! Please note that the unit tests are still failing: https://travis-ci.org/mozilla-b2g/gaia/jobs/18761139

Comment 21

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8370736 [details] [review]
patch for Gaia/master

Hey Mike,

thanks for the feedback was very helpful to understand the Clock App logic, I made the changes and I think that now the PR looks OK :) but if we can do something better it will be great :)

Comment 22

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8370736 [details] [review]
patch for Gaia/master

Hi Pavel,

It looks like the unit tests and the integration tests are failing. Let me know if you need help getting those suites running locally or if you could use a hand interpreting the errors!

Comment 23

[Mike Pennisi [:jugglinmike]]

Hey Pavel,

I won't be able to help debug these specific errors until tomorrow, but in case you want to get a head start, here's some information on how to run the tests locally:

## Unit tests

1. Run the following command in a terminal to create a debugging profile:

    $ DEBUG=1 make

2. Run the following command in a terminal to start the test server:

    $ make test-agent-server &

3. Open Firefox/Nightly with the following command (Replace "/path/to/nightly/" and "/path/to/gaia/" with the correct locations on your system. Don't foget to add "profile-debug" to the end of the Gaia path--that's the profile you built in step 1):

    $ /path/to/nightly -profile /path/to/gaia/profile-debug http://test-agent.gaiamobile.org:8080

4. Run the Clock tests with the following command:

    $ APP=clock make test-agent-test

## Integration tests

1. Run the following command in a terminal:

    $ APP=clock make test-integration

Comment 24

[Mike Pennisi [:jugglinmike]]

Attachment: 918400-fixes.patch

Hi Pavel,

I was able to solve this problem for you. I'm attaching a diff that describes the exact changes necessary. Here's an explanation:

1. A few unit tests expected the `getVibrateSelect` method, which you replaced with `getVibrateCheckbox`. Changing the references fixed that
2. The Timer panel depends on its timer element having the ID `timer-vibrate`. Changing that to `vibrate-checkbox` causes problems in the panel internals, so it's best to leave it alone (since there is no need to modify it to fix this bug).
3. You may remember that when you renamed the Alarm Edit form's vibrate input from "vibrate-select" to "vibrate-switch", I recommended that you use the name "vibrate-checkbox" instead [1]. It looks like you accidentally made this change to the *Timer*'s input. So when you incorporate #2 (above), you'll find that there is no longer an element with the ID "vibrate-checkbox" in the document. You'll have to change the ID on the Alarm Edit panel's vibrate input to "vibrate-checkbox".

Finally, you will notice that this patch does not merge cleanly anymore. That is because we finally merged bug 922128 since you first submitted your pull request. After you incorporate the suggestions I've made above, you may want to look over the changes in that bug--it should help you learn how to update your patch to merge cleanly again.

[1] https://github.com/mozilla-b2g/gaia/pull/15742#discussion_r9722424

Comment 25

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Mike,

now I clearly understand :) so you will create a patch for this bug, right? maybe only my CSS changes need to be addressed?

Comment 26

[Mike Pennisi [:jugglinmike]]

Actually, I attached the patch with comment 24; see attachment 8387338 [details] [diff] [review] . You should be able to apply that to your branch directly. From there, it's just a matter of rebasing over Gaia's current `master` branch.

Comment 27

[Candice Serran (:cserran)]

Has this patch landed? If it is still in progress, we need to update the target milestone to correct sprint.  Is there an ETA for this? Thanks!

Comment 28

[Mike Pennisi [:jugglinmike]]

I can confirm that a patch hasn't landed yet, but I'll leave it to Pavel to give an ETA.

Comment 29

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Guys,

I'm working on this patch now ... I think that I will be done tomorrow.

Comment 30

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8370736 [details] [review]
patch for Gaia/master

Hey Mike,

I create a PR based on your suggestions but still have some problems with test
https://travis-ci.org/mozilla-b2g/gaia/builds/21542612

can you help?

Comment 31

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8370736 [details] [review]
patch for Gaia/master

I'll work with you on this this week, :pivanov.

Comment 32

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18340/files

Pavel, I've posted a new PR with a few minor changes from your patch, mainly removing unnecessary boilerplate and simplifying it a bit. Travis looks like it's going to pass. Feel free to either take it and modify the PR as desired, or give feedback/review it if you'd rather; I'm happy to commit it with you as the author if you don't think any further changes need to be made.

Comment 33

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8407104 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18340/files

Thanks :)

Comment 34

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

But I think you should ask Mike to r+ this one too

Comment 35

[Marcus Cavanaugh [:mcav]]

Given that the patch is functionally the same as what Mike provided and we unfortunately haven't heard from his 'feedback?' for this bug for a few weeks, I'm going to go ahead and land with your r+; he's welcome to weigh in if he resurfaces and has other comments. :)

master: https://github.com/mozilla-b2g/gaia/commit/2a7ef0a971d8e0b36d26d98ad039b6a79bbc4fa7

Comment 36

[Edward Chen[:edchen]]

[Environment]
Gaia      dadf0e60a6421f5b57ee9fc536c6617212805c19
Gecko     https://hg.mozilla.org/mozilla-central/rev/c55dfb01a027
BuildID   20140417160203
Version   31.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013

[Result]
PASS