Closed Bug 867038 Opened 11 years ago Closed 11 years ago

[Clock][User Story] Alarms with selected audible tone and/or vibration

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18+ verified, b2g-v1.1hd fixed)

VERIFIED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
b2g18 + verified
b2g-v1.1hd --- fixed

People

(Reporter: pdol, Assigned: mikehenrty)

References

()

Details

(Keywords: feature, Whiteboard: c= , inarirun2,)

Attachments

(6 files)

UCID: Clock-017

User Story:
As a user, I want to be able to set an alarm using a selected audible tone with vibration, a selected audible tone with no vibration or a vibration with no audible tone so that I can configure how I want to be alerted in different scenarios.
Quick question: Should the user be able to select and set multiple of these (i.e. an alarm with an audible tone and no vibration for my "Wake Up Alarm" but an alarm with no audible tone and vibration only for my "Get up and walk to this meeting" alarm)?
CC'ing Michael Henretty (Developer) so he's aware of any changes or questions about the features expected behavior. I will create 1 or more dependent bugs to track the implementation of this feature's criteria.
Depends on: 867374
Test Case #7761: [Clock] Users can set alarms with selected audible tone and/or vibration
Test case created to cover this new User Story for UCID: Clock-017
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/49093145
See Stephany's questions in comment #1.

When can we finalize on the expected behavior? I'd like to have Michael H start working on this asap.
Flags: needinfo?(pdolanjski)
Flags: in-moztrap?
Flags: in-moztrap?
(In reply to Stephany Wilkes from comment #1)
> Quick question: Should the user be able to select and set multiple of these
> (i.e. an alarm with an audible tone and no vibration for my "Wake Up Alarm"
> but an alarm with no audible tone and vibration only for my "Get up and walk
> to this meeting" alarm)?

That would be ideal.  This is how it currently works for the audible tones.
Flags: needinfo?(pdolanjski)
Sorry about my ignorance, this will be my first actual Gaia feature. 

A couple of questions: what form will the spec for this feature take when it's ready? Will it be comments in this bug, or perhaps in pivotal (which I currently don't have access to the project), or something in moztraps?

Also, do we have an estimate for when the spec for this feature will be ready?
Flags: needinfo?(swilkes)
Hardware: x86 → ARM
Hi Michael,

No worries! I'm pretty new too. You can see the early draft of the spec in the linked bug (867374), at a URL in the comments.

I am flagging Brad on needinfo to ensure that the final spec is also attached to the spec bug.
Flags: needinfo?(swilkes)
Flags: needinfo?(brad)
tracking-b2g18: --- → +
Whiteboard: c=
Keywords: feature
Stephany,

How much longer before UX is ready to hand-off to engineering?

We committed to have this implemented and in (uplifted to) 1.1 by June 7th.
Flags: needinfo?(swilkes)
Brad, can you upload the final? Thanks!
Flags: needinfo?(swilkes)
Flags: needinfo?(brad)
Michael, I've emailed Brad and Shane. Also, if this is high enough priority that engineering is gunning for it to be done for June 7, should it be a candidate for leo? (which is where we got the user story and the requirement)?
Flags: needinfo?(brad)
Stephany,
Yes it's a commitment we've made for Leo.

Michael Treese,
Should we be marking this as a leo+ (blocker) and is there any confidentiality we need to set on this and related bugs?
Flags: needinfo?(mtreese)
Depends on: 855426
Henretty,
This is ready for you to start implementing. See bug 855426 for UX specs.
Assignee: nobody → mhenretty
Status: NEW → ASSIGNED
Flags: needinfo?(brad)
Attached image Mock Up for Edit Alarm
Hi, there's been a request from Sergi for a revision to the mock up for the alarm settings screen.

Sergi, can you confirm this is correct?  Also, can you please let us know when the building blocks are ready for the new label style used in this bug?

Thanks!
Flags: needinfo?(sergiov)
(In reply to Eric Pang [:epang] from comment #14)
> Created attachment 750660 [details]
> Mock Up for Edit Alarm
> 
> Hi, there's been a request from Sergi for a revision to the mock up for the
> alarm settings screen.
> 
> Sergi, can you confirm this is correct?  Also, can you please let us know
> when the building blocks are ready for the new label style used in this bug?
> 
> Thanks!

Perfect!

Thank you very much Eric
Flags: needinfo?(sergiov)
(In reply to Sergi from comment #15)
> (In reply to Eric Pang [:epang] from comment #14)
> > Created attachment 750660 [details]
> > Mock Up for Edit Alarm
> > 
> > Hi, there's been a request from Sergi for a revision to the mock up for the
> > alarm settings screen.
> > 
> > Sergi, can you confirm this is correct?  Also, can you please let us know
> > when the building blocks are ready for the new label style used in this bug?
> > 
> > Thanks!
> 
> Perfect!
> 
> Thank you very much Eric

Great, thanks Sergiy.  Can work be started on this, are the building blocks ready?
Flags: needinfo?(sergiov)
Whiteboard: c= → c=, inarirun2,
scrumbug whiteboard tags (i.e. c= ) need to be separated by a space, appended commas break scrumbu.gs.
Whiteboard: c=, inarirun2, → c= , inarirun2,
blocking-b2g: --- → leo+
Flags: in-moztrap?
Depends on: 872464
Eric, i've been checking the mockup again i'm of the opinion it would make more sense to use a switch to activate/deactivate vibration, instead of using a value selector button. What do you think?
Flags: needinfo?(sergiov) → needinfo?(epang)
Mike,
I have modified the Buttons BB to include labels on it after a request from Pavel: https://bugzilla.mozilla.org/show_bug.cgi?id=872464
As you are now the assignee, please let me know if that change works for you, as by now will be the only app applying that improvement correctly.
You have to use "<ul class="compact">" in your list.

Please I would like to know what works best for your markup:
ul.compact,
ol.compact {
  margin: 0 1.5rem 1.5rem 1.5rem;
}
as it is now, or:
ul.compact,
ol.compact {
  margin: 0 0 1.5rem 0;
}
Thanks
Flags: needinfo?(mhenretty)
Arnau,

I appreciate the changes you have made to the BB, as they have made my markup/css much simpler. In response to your question:

Of the two, I like the first choice because it encourages a common horizontal margin for all compact button lists. However, rather than creating this space as margins, I would prefer using padding instead for two reasons:

1.) ul lists have a default left padding which I have to manually set to 0 (padding-left: 0) in addition to applying the button.css properties to get the intended results. At the least, button.css should set the left padding to 0 for compact lists for me.

2.) It would be nice to be able to set the background color or image of the compact list and have the background take up the entire screen. With margins on the ul list, the background will not cover everything, and I instead need to create a container element and apply the background to that to get the desired effect.

In any case, what we have right now is totally workable, and in the meantime I will use what is there. But if I had my choice, I would use padding :)

Thanks
Flags: needinfo?(mhenretty)
Michael,
Could you please modify buttons BB to change margins to paddings as it works best for you? It will be easier to include it in your patch than creating a new bug to fix this.
You could add me as a reviewer if you want to.
Thanks!
(In reply to arnau from comment #21)
> Michael,
> Could you please modify buttons BB to change margins to paddings as it works
> best for you? It will be easier to include it in your patch than creating a
> new bug to fix this.
> You could add me as a reviewer if you want to.
> Thanks!

Will do, thanks Arnau!
Flags: needinfo?(mtreese)
(In reply to Chris Schmoeckel from comment #3)
> Test Case #7761: [Clock] Users can set alarms with selected audible tone
> and/or vibration
> Test case created to cover this new User Story for UCID: Clock-017
The test cases with tag "clock-017":
https://moztrap.mozilla.org/manage/cases/?&filter-tag=1756
Mike, can you provide the current status on this feature?
Flags: needinfo?(mhenretty)
Hi Dylan, I'll be finishing up work on this in the next day or so, and getting it reviewed by the end of the week. Does that timeline work for you?
Flags: needinfo?(mhenretty)
Yes -- the goal is to have it complete and uplifted to v1-train by 6/3 so that it can be included in the QE3 test cycle, so it sounds like you're on track. Thanks!
Comment on attachment 755704 [details]
https://github.com/mozilla-b2g/gaia/pull/10097

><!doctype html>
><html lang=en>
><head>
><meta charset=utf-8>
><meta http-equiv="Refresh" content="0; url=https://github.com/mozilla-b2g/gaia/pull/10097" />
><title>Redirect to github...</title>
></head>
><body>
><p>Redirecting you to <a href="https://github.com/mozilla-b2g/gaia/pull/10097">https://github.com/mozilla-b2g/gaia/pull/10097</a></p>
></body>
></html>
Attachment #755704 - Attachment mime type: text/plain → text/html
Arnau,

Could you take a look at the building block change? I'll have someone else actually review the changes to the alarm clock, I just want to make sure you're on board with the `/shared/` changes.

It's a small change to /shared/style/buttons.css
https://github.com/mozilla-b2g/gaia/pull/10097/files#diff-8

Thanks
Flags: needinfo?(arnau)
Hi Michael,

If you need a reviewer for functionality with vibration, I can help you on it.
Michael,
Works for me. Thanks!
Flags: needinfo?(arnau)
Ian, given your experience with the clock app, that would be great.
Flags: needinfo?(iliu)
Michael,
It does not work for me after went into each option. I also attache the screen shot for you. My build version is 20130528070209. But I don't think it is relative to Gecko. Maybe I miss some patch?
Flags: needinfo?(iliu)
(In reply to Sergi from comment #18)
> Eric, i've been checking the mockup again i'm of the opinion it would make
> more sense to use a switch to activate/deactivate vibration, instead of
> using a value selector button. What do you think?

Hi Sergi, sorry for the late response.  We decided to use a drop down for the following reasons:
1. This opens itself to further vibration settings in future versions as well, for instnace "vibrate on ring, vibrate on silent" etc.
2. We were also worried the toggle would look out of place with all the other drop downs
Let me know what you think, thanks!
Flags: needinfo?(epang)
(In reply to Eric Pang [:epang] from comment #35)
> (In reply to Sergi from comment #18)
> > Eric, i've been checking the mockup again i'm of the opinion it would make
> > more sense to use a switch to activate/deactivate vibration, instead of
> > using a value selector button. What do you think?
> 
> Hi Sergi, sorry for the late response.  We decided to use a drop down for
> the following reasons:
> 1. This opens itself to further vibration settings in future versions as
> well, for instnace "vibrate on ring, vibrate on silent" etc.
> 2. We were also worried the toggle would look out of place with all the
> other drop downs
> Let me know what you think, thanks!

I see. If the case is like what you point out about having multiple options it may make sense, but, right now, using a value selector to activate/deactivate something looks weird.

Anyway, let's leave this as it is, and maybe it will make more sense in the future when we can display more options.

Thanks!
Attachment #755704 - Flags: review?(gaye)
Attachment #755704 - Flags: review?(gaye)
Mirrored comment from PR:

I love the work you did here. I have some comments, but most of the issues are pretty small. This needs tests though like we discussed, so flag me again on bugzilla when you're ready for another review pass.

Also: if you could commit on top of what you've done and hold off on squashing until we're done with the review, so that it's easy for me to look at the incremental changes you're making, that'd be great. Thanks Mike!
Flags: in-moztrap? → in-moztrap+
Attachment #755704 - Flags: review?(gaye)
Gareth, I added test, and responded to you comments inline. Let me know what you think.
Attachment #755704 - Flags: review?(gaye) → review+
Thanks Mike! You did an *excellent* job; I really appreciate your work here. +++

Landed in master: 02cd2ff72882862c6c707d4250e049f0210414c9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.1 QE2 (6jun)
Don't uplift this yet! mhenretty found an issue that we have to resolve first!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As per :doliver, I opened a new bug to track this problem, and am reclosing this one.

https://bugzilla.mozilla.org/show_bug.cgi?id=879899
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
See Also: → 879899
Uplifted to v1-train.
This also changed the alarm.placeholder string without changing the ID.

That change won't be picked up unless there's a follow up fix.
Thanks for the review Alex. I apologize about my ignorance, can you give me more information about this issue? Do I need to change the data-l10n-id (perhaps to something like "alarmName") before the l10n tools realize there was a change to the placeholder string?
Flags: needinfo?(l10n)
(In reply to Michael Henretty [:mikehenrty] from comment #45)
> Do I need to change the data-l10n-id (perhaps to something like 
> "alarmName") before the l10n tools realize there was a change 
> to the placeholder string?

Exactly. 

By changing the id, an existing localization will be invalidated since the new id is untranslated. If you don't do that, you'll never know if localizers noticed this change and updated their localization.
Flags: needinfo?(l10n)
l10n team, please verify that this patch will update the language string.
Attachment #760528 - Flags: review?(l10n)
Attachment #760528 - Attachment mime type: text/plain → text/html
This is confusing.

In the original patch you changed alarm.placeholder to "Alarm name" (from the original "Alarm").

Now you create a new id (alarmName) and you set it to "Alarm" instead of "Alarm name".
As it is, alarmName.placeholder is never used. So IMO you should get rid of alarmName and set data-l10n-id="alarmName.placeholder"
Correct me if I'm wrong, but I believe the way it works is this: if you want to localize an html attribute (like placeholder, but also things like "alt", or "value") you use this format:

<l10n-id>.<attribute>

So, in this case, I gave my input the "alarmName" id, and then I set the .placeholder string to "Alarm name" in the properties file. If I set my the data-l10n-id to "alarmName.placeholder" as you suggested, the placeholder text doesn't get translated. However, I can remove the alarmName = "Alarm" property, since that never gets used. Would that make it less confusing?

Also, can you point me to some documentation on these property files?
Flags: needinfo?(francesco.lodolo)
Comment on attachment 760528 [details]
Pull Request (https://github.com/mozilla-b2g/gaia/pull/10299)

I have to admit, I don't know the l10n api good enough to review this. Can you ask a gaia peer for review instead?
Attachment #760528 - Flags: review?(l10n) → review?
Attachment #760528 - Flags: review? → review+
This has been manually uplifted to the v1-train as well: https://github.com/mozilla-b2g/gaia/commit/35f34c31fe2dab965e2409b92e7b7307c5b35ba9

We should be good to go here. Nice work Michael!
(In reply to Michael Henretty [:mikehenrty] from comment #49)
> Correct me if I'm wrong, but I believe the way it works is this: if you want
> to localize an html attribute (like placeholder, but also things like "alt",
> or "value") you use this format:
> 
> <l10n-id>.<attribute>

Sorry, my mistake. I never realized that the l10n API was using that syntax to identify attributes. And thanks for removing the unused string.
1.1hd: 35f34c31fe2dab965e2409b92e7b7307c5b35ba9
Hi, all,

Thanks for your help!
I did a quick verification on following patch.
* Test Build:(Mozilla-b2g18_v1_1_0_hd-unagi/2013-07-01-07-02-15)
 + Mercurial-Information:
   - Gecko: "f686f56c11d6"
   - Gaia: ""
 + Git-Information:
   - Gecko: ""
   - Gaia: "c7472acec84f0d4527cdd6fd555d289e1d3e1d1d"

It works as expected.

Test cases:
1. Sound + Vibration -> Pass
2. Sound + No vibration -> Pass
3. No sound + Vibration -> Pass
4. No sound + No vibration -> Pass

Also, attaching the latest UI for your reference.
Status: RESOLVED → VERIFIED
Attached image Create a new alarm
Issue no longer repro on Leo device 
Build ID: 20130807071207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/11bb1b0eef
Gaia: 60ca81600a080dae33058b0692ecaa213556c926
Platform Version: 18.1
Alarm working as expected. 
Sound + Vibration, Sound + No vibration, No sound + Vibration,
No sound + No vibration.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: