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

Issue 9003043: Adds filter controls to the search in the browser

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

Description

Adds filter controls to the search in the browser - The branch updates the models/browser.js Filter model that is basically the search state in the application. - The subapp keeps an instance of that around and updates it along with the rest of the subapplication state. - The subapp passes the raw data down into views that can use it to populate the search related widgets (search and filters). - When those widgets are changed they trigger a custom event back up to their controlling View. - That View then generates a change event to the subapp directing it to update it's current _filter (the Filter model) with that change and navigate to the new url. - All searches are url-able and thus force a navigate and re-process through the render chain. - Because of this, the Views do not need to update their version of the filters, they'll get new ones from the subapp on the next routing. Known QA issues: - There's not a good "no results found" display - There's not a list of selected filters per UX design when the filter list is collapsed in sidebar view - Theme'ing needs to be done on the filter list to remove the leading *, make things more header-like, add proper arrows vs temp ^/v - It's felt these are correctable through the week while the functionality is able to be added before 'feature freeze' on Monday. https://code.launchpad.net/~rharding/juju-gui/search-filters/+merge/161289 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 20

Patch Set 2 : Adds filter controls to the search in the browser #

Patch Set 3 : Adds filter controls to the search in the browser #

Patch Set 4 : Adds filter controls to the search in the browser #

Patch Set 5 : Adds filter controls to the search in the browser #

Patch Set 6 : Adds filter controls to the search in the browser #

Patch Set 7 : Adds filter controls to the search in the browser #

Patch Set 8 : Adds filter controls to the search in the browser #

Total comments: 23

Patch Set 9 : Adds filter controls to the search in the browser #

Patch Set 10 : Adds filter controls to the search in the browser #

Patch Set 11 : Adds filter controls to the search in the browser #

Patch Set 12 : Adds filter controls to the search in the browser #

Patch Set 13 : Adds filter controls to the search in the browser #

Unified diffs Side-by-side diffs Delta from patch set Stats (+828 lines, -195 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
D app/assets/images/contract_icon.jpg View 1 2 3 4 5 6 0 chunks +-1 lines, --1 lines 0 comments Download
A app/assets/images/contract_icon.png View 1 2 3 4 5 6 0 chunks +-1 lines, --1 lines 0 comments Download
D app/assets/images/expand_icon.jpg View 1 2 3 4 5 6 0 chunks +-1 lines, --1 lines 0 comments Download
A app/assets/images/expand_icon.png View 1 2 3 4 5 6 0 chunks +-1 lines, --1 lines 0 comments Download
M app/models/browser.js View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +90 lines, -27 lines 0 comments Download
M app/modules-debug.js View 1 chunk +4 lines, -0 lines 0 comments Download
M app/store/charm.js View 1 1 chunk +3 lines, -3 lines 0 comments Download
M app/subapps/browser/browser.js View 1 2 3 4 5 6 7 8 9 10 7 chunks +21 lines, -9 lines 0 comments Download
M app/subapps/browser/templates/browser_charm.handlebars View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -2 lines 0 comments Download
M app/subapps/browser/templates/search.handlebars View 1 2 3 4 5 6 7 8 1 chunk +20 lines, -3 lines 0 comments Download
M app/subapps/browser/views/search.js View 1 2 3 4 5 6 7 8 9 10 7 chunks +124 lines, -48 lines 0 comments Download
M app/subapps/browser/views/view.js View 5 chunks +43 lines, -31 lines 0 comments Download
M app/templates/browser-search.handlebars View 1 chunk +1 line, -1 line 0 comments Download
M app/templates/charm-container.handlebars View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
A app/templates/filters.handlebars View 1 2 3 4 5 6 7 8 9 1 chunk +47 lines, -0 lines 0 comments Download
M app/widgets/charm-search.js View 1 5 chunks +34 lines, -25 lines 0 comments Download
A app/widgets/filter.js View 1 2 3 4 5 6 7 8 9 1 chunk +258 lines, -0 lines 0 comments Download
M lib/views/browser/main.less View 1 2 3 4 5 6 7 8 2 chunks +47 lines, -4 lines 0 comments Download
M test/index.html View 2 chunks +2 lines, -1 line 0 comments Download
M test/test_browser_models.js View 1 2 2 chunks +25 lines, -16 lines 0 comments Download
M test/test_browser_search_view.js View 2 chunks +4 lines, -2 lines 0 comments Download
M test/test_browser_search_widget.js View 2 chunks +3 lines, -24 lines 0 comments Download
M test/test_charm_store.js View 1 chunk +1 line, -1 line 0 comments Download
A test/test_filter_widget.js View 1 2 3 4 5 1 chunk +99 lines, -0 lines 0 comments Download

Messages

Total messages: 18
rharding
Please take a look.
10 years, 11 months ago (2013-04-28 14:45:56 UTC) #1
rharding
Publishing first sets of comments up to the filter widget file. https://codereview.appspot.com/9003043/diff/1/app/config-debug.js File app/config-debug.js (right): ...
10 years, 11 months ago (2013-04-28 16:25:39 UTC) #2
rharding
Finish the last of the pre-review review. https://codereview.appspot.com/9003043/diff/1/app/widgets/filter.js File app/widgets/filter.js (right): https://codereview.appspot.com/9003043/diff/1/app/widgets/filter.js#newcode45 app/widgets/filter.js:45: if (target.get('checked')) ...
10 years, 11 months ago (2013-04-28 18:29:10 UTC) #3
rharding
Please take a look.
10 years, 11 months ago (2013-04-28 19:12:53 UTC) #4
rharding
Please take a look.
10 years, 11 months ago (2013-04-28 19:29:35 UTC) #5
rharding
On 2013/04/28 19:29:35, rharding wrote: > Please take a look. Major apologies for the size ...
10 years, 11 months ago (2013-04-28 19:31:59 UTC) #6
rharding
Please take a look.
10 years, 11 months ago (2013-04-28 19:35:59 UTC) #7
rharding
Please take a look.
10 years, 11 months ago (2013-04-28 20:15:28 UTC) #8
rharding
Please take a look.
10 years, 11 months ago (2013-04-28 20:27:05 UTC) #9
rharding
Please take a look.
10 years, 11 months ago (2013-04-29 11:09:51 UTC) #10
rharding
Please take a look.
10 years, 11 months ago (2013-04-29 12:00:19 UTC) #11
j.c.sackett
https://codereview.appspot.com/9003043/diff/25001/app/config-debug.js File app/config-debug.js (right): https://codereview.appspot.com/9003043/diff/25001/app/config-debug.js#newcode15 app/config-debug.js:15: charmworldURL: 'http://ec2-75-101-184-138.compute-1.amazonaws.com:6543/', Is this intentional? Seems to be something ...
10 years, 11 months ago (2013-04-29 17:04:40 UTC) #12
rharding
Comments below. Thanks for the notes. https://codereview.appspot.com/9003043/diff/25001/app/config-debug.js File app/config-debug.js (right): https://codereview.appspot.com/9003043/diff/25001/app/config-debug.js#newcode15 app/config-debug.js:15: charmworldURL: 'http://ec2-75-101-184-138.compute-1.amazonaws.com:6543/', On ...
10 years, 11 months ago (2013-04-29 17:13:44 UTC) #13
rharding
Please take a look.
10 years, 11 months ago (2013-04-29 18:42:30 UTC) #14
j.c.sackett
LGTM -- thanks for those changes
10 years, 11 months ago (2013-04-29 18:50:47 UTC) #15
benji
This branch looks good. I didn't see anything major. You may want to add in ...
10 years, 11 months ago (2013-04-29 20:18:08 UTC) #16
rharding
Thanks for the review. Updating as requested. https://codereview.appspot.com/9003043/diff/25001/app/models/browser.js File app/models/browser.js (right): https://codereview.appspot.com/9003043/diff/25001/app/models/browser.js#newcode141 app/models/browser.js:141: @param {Object} ...
10 years, 11 months ago (2013-04-29 20:24:59 UTC) #17
rharding
10 years, 11 months ago (2013-04-29 21:18:51 UTC) #18
*** Submitted:

Adds filter controls to the search in the browser

- The branch updates the models/browser.js Filter model that is basically the
search state in the application.
- The subapp keeps an instance of that around and updates it along with the
rest of the subapplication state.
- The subapp passes the raw data down into views that can use it to populate
the search related widgets (search and filters).
- When those widgets are changed they trigger a custom event back up to their
controlling View. 
- That View then generates a change event to the subapp directing it to update
it's current _filter (the Filter model) with that change and navigate to the
new url.
- All searches are url-able and thus force a navigate and re-process through
the render chain.
- Because of this, the Views do not need to update their version of the
filters, they'll get new ones from the subapp on the next routing.

Known QA issues:

- There's not a good "no results found" display
- There's not a list of selected filters per UX design when the filter list is
collapsed in sidebar view
- Theme'ing needs to be done on the filter list to remove the leading *, make
things more header-like, add proper arrows vs temp ^/v
- It's felt these are correctable through the week while the functionality is
able to be added before 'feature freeze' on Monday.

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

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