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

Issue 10416044: Add support for related charm loading and display

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 3 months ago by rharding
Modified:
6 years, 3 months ago
Reviewers:
mp+170313, teknico, j.c.sackett
Visibility:
Public.

Description

Add support for related charm loading and display - Adds a new store endpoint for the related charm api call - Adds the idea of relatedCharms for model ATTRS - Adds two helpers to take the api response data into something we can use in the app - Renders out the related charms in the template in fullscreen as an ajax request - Updates the charm details to handle click events on charm tokens - Adds tests for the model helpers and the details view - Updates a bunch of tests. There were left over testing Nodes that caused stray tests to fail. I fixed those and also went back and cleaned up the stray test mess. - Updates the css to work out so that the tokens can fit. There's a redesign-a-coming so the work was to fit in currently and the redesign will move the layout at a later date. Notes: - This does not currently add the related charm display to the interfaces tab. It's a slightly different UX and this branch was taking too long as is. - I'm not happy with the relatedCharms ATTR. It's not really an ATTR but I wanted this cached into the model because we'll use that when we add related charms to the interfaces tab. We don't want to re-request the data. However, in sidebar move it will not have been requested yet. My plan is to re-evaluate and possibly refactor when doing the interfaces tab display. https://code.launchpad.net/~rharding/juju-gui/related-charm/+merge/170313 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 17

Patch Set 2 : Add support for related charm loading and display #

Patch Set 3 : Add support for related charm loading and display #

Patch Set 4 : Add support for related charm loading and display #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1305 lines, -38 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M app/models/charm.js View 1 2 4 chunks +96 lines, -4 lines 0 comments Download
M app/store/charm.js View 1 chunk +19 lines, -0 lines 0 comments Download
M app/subapps/browser/templates/browser_charm.handlebars View 3 chunks +6 lines, -2 lines 0 comments Download
M app/subapps/browser/views/charm.js View 1 2 7 chunks +91 lines, -9 lines 0 comments Download
M app/subapps/browser/views/editorial.js View 1 chunk +0 lines, -1 line 0 comments Download
M lib/views/browser/content-sidebar.less View 1 chunk +16 lines, -0 lines 0 comments Download
A test/data/related.json View 1 chunk +962 lines, -0 lines 0 comments Download
M test/test_browser_charm_details.js View 1 2 11 chunks +59 lines, -6 lines 0 comments Download
M test/test_browser_search_view.js View 1 chunk +3 lines, -2 lines 0 comments Download
M test/test_charm_store.js View 1 1 chunk +1 line, -1 line 0 comments Download
M test/test_charm_token.js View 3 chunks +3 lines, -3 lines 0 comments Download
M test/test_model.js View 1 4 chunks +43 lines, -6 lines 0 comments Download
M test/test_resizing_textarea.js View 2 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 10
rharding
Please take a look.
6 years, 3 months ago (2013-06-19 17:34:31 UTC) #1
rharding
found one typo already wheeee https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/charm.js File app/subapps/browser/views/charm.js (right): https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/charm.js#newcode514 app/subapps/browser/views/charm.js:514: if (this.featuredCharmContainer) { this ...
6 years, 3 months ago (2013-06-19 17:39:40 UTC) #2
teknico
LGTM, did not go through the big JSON file, as instructed. :-) Please find some ...
6 years, 3 months ago (2013-06-20 14:17:36 UTC) #3
rharding
Thanks for the review. Questions hopefully answered and will push an update shortly. https://codereview.appspot.com/10416044/diff/1/app/subapps/browser/views/charm.js File ...
6 years, 3 months ago (2013-06-20 14:22:23 UTC) #4
rharding
Please take a look.
6 years, 3 months ago (2013-06-20 14:27:48 UTC) #5
j.c.sackett
https://codereview.appspot.com/10416044/diff/1/app/models/charm.js File app/models/charm.js (right): https://codereview.appspot.com/10416044/diff/1/app/models/charm.js#newcode391 app/models/charm.js:391: shouldShowIcon: data.has_icon, Related charms aren't inherently reviewed, are they? ...
6 years, 3 months ago (2013-06-20 14:28:17 UTC) #6
rharding
Please take a look. https://codereview.appspot.com/10416044/diff/1/test/test_model.js File test/test_model.js (right): https://codereview.appspot.com/10416044/diff/1/test/test_model.js#newcode740 test/test_model.js:740: // This must be converted ...
6 years, 3 months ago (2013-06-20 14:51:58 UTC) #7
rharding
On 2013/06/20 14:28:17, j.c.sackett wrote: > https://codereview.appspot.com/10416044/diff/1/app/models/charm.js > File app/models/charm.js (right): > > https://codereview.appspot.com/10416044/diff/1/app/models/charm.js#newcode391 > ...
6 years, 3 months ago (2013-06-20 14:52:02 UTC) #8
j.c.sackett
LGTM, thanks. On 2013/06/20 14:52:02, rharding wrote: > I thought it was messier but very ...
6 years, 3 months ago (2013-06-20 16:02:31 UTC) #9
rharding
6 years, 3 months ago (2013-06-21 11:05:23 UTC) #10
*** Submitted:

Add support for related charm loading and display

- Adds a new store endpoint for the related charm api call
- Adds the idea of relatedCharms for model ATTRS
- Adds two helpers to take the api response data into something we can use in
the app
- Renders out the related charms in the template in fullscreen as an ajax
request
- Updates the charm details to handle click events on charm tokens 
- Adds tests for the model helpers and the details view
- Updates a bunch of tests. There were left over testing Nodes that caused
stray tests to fail. I fixed those and also went back and cleaned up the stray
test mess.
- Updates the css to work out so that the tokens can fit. There's a
redesign-a-coming so the work was to fit in currently and the redesign will
move the layout at a later date.

Notes:

- This does not currently add the related charm display to the interfaces tab.
It's a slightly different UX and this branch was taking too long as is.

- I'm not happy with the relatedCharms ATTR. It's not really an ATTR but I
wanted this cached into the model because we'll use that when we add related
charms to the interfaces tab. We don't want to re-request the data. However,
in sidebar move it will not have been requested yet. My plan is to re-evaluate
and possibly refactor when doing the interfaces tab display.

R=
CC=
https://codereview.appspot.com/10416044
Sign in to reply to this message.

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