Closed Bug 909336 Opened 8 years ago Closed 8 years ago

[Clock] Implement UI for switching views

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jugglinmike, Assigned: gnarf)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → gnarf37
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #800352 - Flags: review?(mike)
Also submitted as a pull request for travis purposes: https://github.com/mozilla-b2g/gaia/pull/11969
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).
Attachment #800352 - Flags: review?(mike)
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #800352 - Attachment is obsolete: true
Attachment #800871 - Flags: review?(mike)
Attached patch patch v3 (obsolete) — Splinter Review
fixes problem with alarm checkbox toggle
Attachment #800871 - Attachment is obsolete: true
Attachment #800871 - Flags: review?(mike)
Attachment #800899 - Flags: review?(mike)
Attached patch patch v4 (obsolete) — Splinter Review
renamed the outer view containers to be -panel
Attachment #800899 - Attachment is obsolete: true
Attachment #800899 - Flags: review?(mike)
Attachment #800941 - Flags: review?(mike)
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
Attachment #800941 - Flags: review?(mike)
Attached patch patch v5Splinter Review
Adds a multi-stage loading system to fix the noted problem
Attachment #800941 - Attachment is obsolete: true
Attachment #801161 - Flags: review?(mike)
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 :)
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?
Flags: needinfo?(gnarf37)
(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
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
Flags: needinfo?(bob.silverberg)
I've updated my branch on github to contain the rebase fix from comment 10.  LGTM
Flags: needinfo?(gnarf37)
The gaia-ui-tests pull request has been merged.
Flags: needinfo?(bob.silverberg)
Comment on attachment 801161 [details] [diff] [review]
patch v5

Very nice work, Corey!
Attachment #801161 - Flags: review?(mike) → review+
master: https://github.com/mozilla-b2g/gaia/commit/ae0a33c2f0e7258f1dba7dc39bfc2ba500561c1c

Thanks Corey! And thanks, Bob!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.