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

Issue 8726048: Support linking through router code for urls.

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by rharding
Modified:
9 years, 7 months ago
Reviewers:
curtis, jeff.pihach, mp+159215
Visibility:
Public.

Description

Support linking through router code for urls. - Add state tracking to the subapp and use that to determine which UX parts need calling and which do not. - Add an event to the subapp and views to handle catching requests to route to a new url. - Add tests to walk the paths of rendering to make sure we're updating and calling correctly. - Add some stubs for search, but it's to be implemented in follow up. https://code.launchpad.net/~rharding/juju-gui/browser_links/+merge/159215 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : Support linking through router code for urls. #

Total comments: 7

Patch Set 3 : Support linking through router code for urls. #

Total comments: 8

Patch Set 4 : Support linking through router code for urls. #

Total comments: 7

Patch Set 5 : Support linking through router code for urls. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+711 lines, -165 lines) Patch
A [revision details] View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M app/subapps/browser/browser.js View 1 2 3 4 7 chunks +323 lines, -137 lines 0 comments Download
M app/subapps/browser/templates/browser_charm.handlebars View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M app/subapps/browser/views/charm.js View 1 2 3 3 chunks +20 lines, -1 line 0 comments Download
M app/subapps/browser/views/editorial.js View 1 2 3 4 4 chunks +57 lines, -8 lines 0 comments Download
M app/subapps/browser/views/view.js View 3 chunks +23 lines, -1 line 0 comments Download
M app/templates/browser-search.handlebars View 1 chunk +2 lines, -4 lines 0 comments Download
M app/templates/charm-token.handlebars View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M test/test_browser_app.js View 1 2 3 4 1 chunk +282 lines, -12 lines 0 comments Download

Messages

Total messages: 10
rharding
Please take a look.
9 years, 7 months ago (2013-04-17 19:10:57 UTC) #1
rharding
Please take a look.
9 years, 7 months ago (2013-04-17 19:20:23 UTC) #2
j.c.sackett
Rick, several comments below, and really I do think this probably needs tests to land, ...
9 years, 7 months ago (2013-04-17 21:49:22 UTC) #3
rharding
Please take a look.
9 years, 7 months ago (2013-04-19 14:57:58 UTC) #4
curtis
Hi Rick. I have a few questions. Oh, and the remark about setStyle() is a ...
9 years, 7 months ago (2013-04-19 15:36:54 UTC) #5
rharding
Feedback below. https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js File app/subapps/browser/browser.js (right): https://codereview.appspot.com/8726048/diff/9001/app/subapps/browser/browser.js#newcode113 app/subapps/browser/browser.js:113: viewmode: undefined, On 2013/04/19 15:36:54, curtis wrote: ...
9 years, 7 months ago (2013-04-19 15:43:23 UTC) #6
rharding
Please take a look.
9 years, 7 months ago (2013-04-19 15:55:39 UTC) #7
curtis
Thank's Rick. LGTM.
9 years, 7 months ago (2013-04-19 15:58:13 UTC) #8
jeff.pihach
LGTM - There are a few rename and documentation requests below but good work, thanks ...
9 years, 7 months ago (2013-04-19 16:29:45 UTC) #9
rharding
9 years, 7 months ago (2013-04-19 17:13:56 UTC) #10
*** Submitted:

Support linking through router code for urls.

- Add state tracking to the subapp and use that to determine which UX parts
need calling and which do not.
- Add an event to the subapp and views to handle catching requests to route to
a new url.
- Add tests to walk the paths of rendering to make sure we're updating and
calling correctly.
- Add some stubs for search, but it's to be implemented in follow up.

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

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