Closed
Bug 909336
Opened 11 years ago
Closed 11 years ago
[Clock] Implement UI for switching views
Categories
(Firefox OS Graveyard :: Gaia::Clock, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jugglinmike, Assigned: gnarf)
References
Details
Attachments
(1 file, 4 obsolete files)
42.06 KB,
patch
|
jugglinmike
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → gnarf37
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #800352 -
Flags: review?(mike)
Assignee | ||
Comment 2•11 years ago
|
||
Also submitted as a pull request for travis purposes: https://github.com/mozilla-b2g/gaia/pull/11969
Reporter | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
fixes problem with alarm checkbox toggle
Attachment #800871 -
Attachment is obsolete: true
Attachment #800871 -
Flags: review?(mike)
Attachment #800899 -
Flags: review?(mike)
Assignee | ||
Comment 6•11 years ago
|
||
renamed the outer view containers to be -panel
Attachment #800899 -
Attachment is obsolete: true
Attachment #800899 -
Flags: review?(mike)
Attachment #800941 -
Flags: review?(mike)
Reporter | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
Adds a multi-stage loading system to fix the noted problem
Attachment #800941 -
Attachment is obsolete: true
Attachment #801161 -
Flags: review?(mike)
Comment 9•11 years ago
|
||
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 :)
Reporter | ||
Comment 10•11 years ago
|
||
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)
Reporter | ||
Comment 11•11 years ago
|
||
(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
Reporter | ||
Comment 12•11 years ago
|
||
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)
Assignee | ||
Comment 13•11 years ago
|
||
I've updated my branch on github to contain the rebase fix from comment 10. LGTM
Flags: needinfo?(gnarf37)
Comment 14•11 years ago
|
||
The gaia-ui-tests pull request has been merged.
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 15•11 years ago
|
||
Comment on attachment 801161 [details] [diff] [review] patch v5 Very nice work, Corey!
Attachment #801161 -
Flags: review?(mike) → review+
Reporter | ||
Comment 16•11 years ago
|
||
master: https://github.com/mozilla-b2g/gaia/commit/ae0a33c2f0e7258f1dba7dc39bfc2ba500561c1c Thanks Corey! And thanks, Bob!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•