913468 - [Clock] Refactor Banner Notification

Status: RESOLVED FIXED

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

Description

[Adam Hyland [:protonk]]

Refactoring of banner.js which started in Bug 832335 to resolve UX issues. The UX discussion is independent of the refactoring.

Comment 1

[Adam Hyland [:protonk]]

Attachment: Pull request on github

Comment 2

[Mike Pennisi [:jugglinmike]]

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

Hey Adam,

I've left some specific feedback on the GitHub pull request. More generally, I think we can re-factor this a little further to make it more modular and more testable.

The biggest concern right now is the reference to `document` (and the hard-coded selector). If we generalize `Banner` to be initialized with a DOM node, we can test it easier and "push" the DOM selector (which is more of an application-level concern) higher into the application.

The `alarmTime` is really more of a rendering concern, so I think we should specify it to `show` directly. Then we could create the banner before we render it.

Here's what I'm thinking:

    // AlarmList.init:
    this.banner = new Banner(document.getElementById('banner-countdown'));

    // AlarmEdit.save
    AlarmList.banner.show(alarm.getNextAlarmFireTime());

Comment 3

[Adam Hyland [:protonk]]

(In reply to Mike Pennisi [:jugglinmike] from comment #2)
> Comment on attachment 800784 [details] [review]
> Pull request on github
> 
> Hey Adam,
> 
> I've left some specific feedback on the GitHub pull request. More generally,
> I think we can re-factor this a little further to make it more modular and
> more testable.
> 
> The biggest concern right now is the reference to `document` (and the
> hard-coded selector). If we generalize `Banner` to be initialized with a DOM
> node, we can test it easier and "push" the DOM selector (which is more of an
> application-level concern) higher into the application.
> 
> The `alarmTime` is really more of a rendering concern, so I think we should
> specify it to `show` directly. Then we could create the banner before we
> render it.
> 
> Here's what I'm thinking:
> 
>     // AlarmList.init:
>     this.banner = new Banner(document.getElementById('banner-countdown'));
> 
>     // AlarmEdit.save
>     AlarmList.banner.show(alarm.getNextAlarmFireTime());

Does it make sense to simply pass two arguments to Banner()? I like the idea of instantiating a new banner and then immediately forgetting it as an indication to others that this isn't really a persistent element.

Comment 4

[Mike Pennisi [:jugglinmike]]

(In reply to Adam Hyland [:protonk] from comment #3)
> Does it make sense to simply pass two arguments to Banner()? I like the idea
> of instantiating a new banner and then immediately forgetting it as an
> indication to others that this isn't really a persistent element.

The element does persist. This approach makes it more clear that the element hangs around and isn't removed from the DOM. Dealing with a `Banner` instance also justifies the implementation as a class (and not simply a `doBannerStuff` function).

Comment 5

[Adam Hyland [:protonk]]

(In reply to Mike Pennisi [:jugglinmike] from comment #4)
> (In reply to Adam Hyland [:protonk] from comment #3)
> > Does it make sense to simply pass two arguments to Banner()? I like the idea
> > of instantiating a new banner and then immediately forgetting it as an
> > indication to others that this isn't really a persistent element.
> 
> The element does persist. This approach makes it more clear that the element
> hangs around and isn't removed from the DOM. Dealing with a `Banner`
> instance also justifies the implementation as a class (and not simply a
> `doBannerStuff` function).

Sounds reasonable. Putting the call to Banner on AlarmList.init means a few more unit tests need to be updated (at least for now) but the new way separates the element handling, localization and rendering nicely.

Comment 6

[Mike Pennisi [:jugglinmike]]

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

Adam: Hello.

Now that we've landed Corey's patch for bug 909336, this patch has two trivial conflicts with `master` (isolated to `index.html`). Also: you'll need to add `js/banner.js` file to the `loadQueue` data structure in the new `js/startup.js` to preserve functionality on the desktop.

Would you mind making these changes and re-flagging me for review when you're ready?

Comment 7

[Adam Hyland [:protonk]]

Ok Mike,

I've updated the patch and rebased on top of Corey's change. Should be good to review.

(In reply to Mike Pennisi [:jugglinmike] from comment #6)
> Comment on attachment 800784 [details] [review]
> Pull request on github
> 
> Adam: Hello.
> 
> Now that we've landed Corey's patch for bug 909336, this patch has two
> trivial conflicts with `master` (isolated to `index.html`). Also: you'll
> need to add `js/banner.js` file to the `loadQueue` data structure in the new
> `js/startup.js` to preserve functionality on the desktop.
> 
> Would you mind making these changes and re-flagging me for review when
> you're ready?

Comment 8

[Mike Pennisi [:jugglinmike]]

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

Nice work, Adam! Thanks for sticking with it :) and :O and :)

Comment 9

[Mike Pennisi [:jugglinmike]]

master: https://github.com/mozilla-b2g/gaia/commit/13d38a3c61034501f0de108dd62bd7f4818e6e7b