909336 - [Clock] Implement UI for switching views

Status: RESOLVED FIXED

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

Description

[Mike Pennisi [:jugglinmike]]

The UI specification for v1.2 contains multiple views (Alarm, Stop Watch, and Timer). Implement the UI for navigating between these views so that each view may be developed in parallel.

Comment 1

[Corey Frang [:gnarf]]

Attachment: patch v1

Comment 2

[Corey Frang [:gnarf]]

Also submitted as a pull request for travis purposes: https://github.com/mozilla-b2g/gaia/pull/11969

Comment 3

[Mike Pennisi [:jugglinmike]]

Comment on attachment 800352 [details] [diff] [review]
patch v1

Review of attachment 800352 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Corey,

This is looking great! I'm excited to integrate this into the application. Two high-level things:

1. According to the visual specifications, the tabs should be overlaid on top of the underlying panel
2. I can't reproduce in Nightly, but on my device, the analog clock resizing is a little bit off: when I have more than 1 alarm, it is rendered large enough to be partially obstructed by the alarm list. My cursory attempts to debug haven't been fruitful, but I wanted to check with you before investigating too thoroughly.

::: apps/clock/index.html
@@ +29,5 @@
>  
>      <!-- Specific code -->
>      <script defer src="shared/js/lazy_loader.js"></script>
> +    <script defer src="js/startup.js"></script>
> +    <!-- Lazy Loaded Scripts - Include here for build process 

There's some trailing white space here

::: apps/clock/js/panel.js
@@ +11,5 @@
> + *
> + * @constructor
> + * @param {HTMLElement} element The element to wrap.
> + */
> +function Panel(element) {

I'm really happy to see this and the View class being initialized with an element reference. Clearly you've written some unit tests in your day :)

@@ +12,5 @@
> + * @constructor
> + * @param {HTMLElement} element The element to wrap.
> + */
> +function Panel(element) {
> +  View.call(this, element);

I think this pattern would be slightly more future-proof if we `apply`d the `arguments` instead. Of course, it will be functionally identical today, but I'm curious about your thoughts on this.

@@ +64,5 @@
> +      return value;
> +    }
> +  },
> +  /**
> +   * Panel.prototype.tranisition - String or false

"Panel.prototype.tranisition" -> "Panel.prototype.transition"

::: apps/clock/js/startup.js
@@ +23,5 @@
> +  'js/constants.js',
> +  'js/utils.js',
> +  'js/alarm.js',
> +  'js/active_alarm.js',
> +  'js/alarmsdb.js',

It looks like we'll have to do a two-step script loading process here, because `alarmsdb.js` has an immediate dependency on `utils.js` (load order is not guaranteed when running in Nightly).

Comment 4

[Corey Frang [:gnarf]]

Attachment: patch v2

Addresses concerns with alarm list overflowing on top of the clock, as well as other nits in Panel.js

The transparency issue isn't solved here yet, most likely I will put this in in another ticket/branch because I really want UX to try using it before they agree thats how they want it.

Comment 5

[Corey Frang [:gnarf]]

Attachment: patch v3

fixes problem with alarm checkbox toggle

Comment 6

[Corey Frang [:gnarf]]

Attachment: patch v4

renamed the outer view containers to be -panel

Comment 7

[Mike Pennisi [:jugglinmike]]

Comment on attachment 800941 [details] [diff] [review]
patch v4

In my earlier review, I mentioned that we need to use a two-step loading
process to ensure that `utils.js` is loaded before `alarmsdb.js` when running
in Nightly. This is necessary because LazyLoader does not set `async=false` on
the generated script tags, and Firefox has not guaranteed execution order of
such scripts since version 3.6 [1].

Since `panel.js` has an immmediate dependency on `view.js`, we should defer
loading `panel.js` until this new second "phase" as well.

The reason some of us have been intermittently reporting errors (and why Evelyn
was able to resolve the issue by cherry-picking her work on top of your branch)
is that your branch is currently behind upstream's `master`. Yesterday, we
landed code that added some methods to `utils.js`. This increased the file size
of `utils.js` by a small amount and triggered the race condition we've been
experiencing when rebasing your patch.

When I first formed this theory, I was a little incredulous. I convinced myself
by checking out your branch (*without* rebasing), verifying correct behavior,
adding a bunch of lines of JavaScript comments to `utils.js` (7k's worth did it
for me), and witnessing the ReferenceError. Implementing a second loading
"phase" for `alarmsdb.js` resolved the problem.

Kind of makes you wonder why someone hasn't written some sort of dependency
management library in JavaScript, doesn't it?

[1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

Comment 8

[Corey Frang [:gnarf]]

Attachment: patch v5

Adds a multi-stage loading system to fix the noted problem

Comment 9

[Rick Waldron [:rwaldron]]

Comment on attachment 801161 [details] [diff] [review]
patch v5

Review of attachment 801161 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/clock/js/startup.js
@@ +59,5 @@
> +    }
> +    navigator.mozL10n.ready(initialize);
> +  }
> +}
> +

Clever :)

Comment 10

[Mike Pennisi [:jugglinmike]]

Hey Corey.

Recent upstream changes (in service of bug) prevent this patch from merging cleanly. There is one conflict in `index.html`. The resolution is pretty straightforward, but I'd like your feedback before I alter and land this patch. The conflic:

    <<<<<<< HEAD
      <a href="#alarm" class="alarm-item${withRepeat}" data-id="${id}">
    =======
      <a href="#alarm-edit-panel" class="alarm-item" data-id="${id}">
    >>>>>>> Bug 909336 - [Clock] Implement UI for switching views

My resolution:

      <a href="#alarm-edit-panel" class="alarm-item${withRepeat}" data-id="${id}">

Look good to you?

Comment 11

[Mike Pennisi [:jugglinmike]]

(In reply to Mike Pennisi [:jugglinmike] from comment #10)
> Hey Corey.
> 
> Recent upstream changes (in service of bug) prevent this patch from merging
> cleanly.

I meant to reference bug 910254

Comment 12

[Mike Pennisi [:jugglinmike]]

This patch looks good to land! Unfortunately, it is currently failing the `gaia-ui-tests` QA suite. The issue is isolated to the test suite [1], so no further changes are needed here.

In order to keep the tests passing, we will need to wait until the issue is resolved. I wrote a patch [2]  with Bob Silverberg's help, and I'm hopeful this will be accepted once the team has a change to review it. Since that project does not use BugZilla, I am going to approximate the "Dependency" attribute by flagging Bob with "needsinfo".

[1] https://github.com/mozilla/gaia-ui-tests/issues/1346
[2] https://github.com/mozilla/gaia-ui-tests/pull/1347

Comment 13

[Corey Frang [:gnarf]]

I've updated my branch on github to contain the rebase fix from comment 10.  LGTM

Comment 14

[Bob Silverberg [:bsilverberg]]

The gaia-ui-tests pull request has been merged.

Comment 15

[Mike Pennisi [:jugglinmike]]

Comment on attachment 801161 [details] [diff] [review]
patch v5

Very nice work, Corey!

Comment 16

[Mike Pennisi [:jugglinmike]]

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

Thanks Corey! And thanks, Bob!