Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(717)

Issue 8937046: Auto hide the browser when in :gui: views

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by rharding
Modified:
10 years, 11 months ago
Reviewers:
mp+160993, j.c.sackett, jeff.pihach
Visibility:
Public.

Description

Auto hide the browser when in :gui: views - Add a route/middleware to always check if the browser app should be hidden. - If so, mark it hidden and force the browser app to re-update it's visible state. - Add the helpers for this in the browser subapp by adding the hidden attribute, a updateVisible() helper, and tests that routing is bypassed when hidden. - Some existing tests cover refactoring the hidden dom changes https://code.launchpad.net/~rharding/juju-gui/hide-me/+merge/160993 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 10

Patch Set 2 : Auto hide the browser when in :gui: views #

Patch Set 3 : Auto hide the browser when in :gui: views #

Patch Set 4 : Auto hide the browser when in :gui: views #

Unified diffs Side-by-side diffs Delta from patch set Stats (+155 lines, -12 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M app/app.js View 1 2 chunks +31 lines, -0 lines 0 comments Download
M app/subapps/browser/browser.js View 6 chunks +62 lines, -9 lines 0 comments Download
M test/test_app.js View 1 2 3 2 chunks +36 lines, -1 line 0 comments Download
M test/test_browser_app.js View 1 2 chunks +24 lines, -2 lines 0 comments Download

Messages

Total messages: 7
rharding
Please take a look.
10 years, 11 months ago (2013-04-25 19:46:02 UTC) #1
jeff.pihach
Thanks this looks great thanks for putting it together! I have a few comments below ...
10 years, 11 months ago (2013-04-25 20:00:05 UTC) #2
j.c.sackett
LGTM, one possible change below. https://codereview.appspot.com/8937046/diff/1/app/app.js File app/app.js (right): https://codereview.appspot.com/8937046/diff/1/app/app.js#newcode796 app/app.js:796: if (url.match(match) || url.match('logout')) ...
10 years, 11 months ago (2013-04-25 20:21:20 UTC) #3
rharding
Replies below. Updated code coming in a bit. https://codereview.appspot.com/8937046/diff/1/app/app.js File app/app.js (right): https://codereview.appspot.com/8937046/diff/1/app/app.js#newcode789 app/app.js:789: showBrowser: ...
10 years, 11 months ago (2013-04-25 21:21:13 UTC) #4
rharding
Please take a look.
10 years, 11 months ago (2013-04-25 21:40:17 UTC) #5
jeff.pihach
LGTM Thanks for the fixes and the extra test!
10 years, 11 months ago (2013-04-25 21:44:43 UTC) #6
rharding
10 years, 11 months ago (2013-04-25 22:09:41 UTC) #7
*** Submitted:

Auto hide the browser when in :gui: views

- Add a route/middleware to always check if the browser app should be hidden.
- If so, mark it hidden and force the browser app to re-update it's visible
state.
- Add the helpers for this in the browser subapp by adding the hidden
attribute, a updateVisible() helper, and tests that routing is bypassed when
hidden.
- Some existing tests cover refactoring the hidden dom changes

R=jeff.pihach, j.c.sackett
CC=
https://codereview.appspot.com/8937046
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b