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

Issue 11499044: Pull the default icon for local charms.

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by rharding
Modified:
10 years, 8 months ago
Reviewers:
mp+176002, benji, gary.poster
Visibility:
Public.

Description

Pull the default icon for local charms. - Local charms do not exist on the back end api server so no icon is available. The charm is 404'd. - Local charms do not expose the category data so we cannot look for a fallback icon. - This branch just forces local charms to have the default charm icon via a hard coded path in the store's charm icon url helper. https://code.launchpad.net/~rharding/juju-gui/local-default-icon/+merge/176002 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 5

Patch Set 2 : Pull the default icon for local charms. #

Total comments: 3

Patch Set 3 : Pull the default icon for local charms. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -28 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M app/store/charm.js View 1 2 chunks +23 lines, -5 lines 0 comments Download
M app/views/utils.js View 1 1 chunk +1 line, -6 lines 0 comments Download
M test/test_charm_store.js View 1 1 chunk +24 lines, -0 lines 0 comments Download
M test/test_environment_view.js View 1 chunk +58 lines, -17 lines 0 comments Download

Messages

Total messages: 8
rharding
Please take a look.
10 years, 8 months ago (2013-07-20 12:42:45 UTC) #1
gary.poster
LGTM, with consideration of comments. Thank you! https://codereview.appspot.com/11499044/diff/1/app/store/charm.js File app/store/charm.js (right): https://codereview.appspot.com/11499044/diff/1/app/store/charm.js#newcode355 app/store/charm.js:355: // icon. ...
10 years, 8 months ago (2013-07-20 21:39:55 UTC) #2
rharding
Comments below. I'll push a small update monday morning. https://codereview.appspot.com/11499044/diff/1/app/store/charm.js File app/store/charm.js (right): https://codereview.appspot.com/11499044/diff/1/app/store/charm.js#newcode355 app/store/charm.js:355: ...
10 years, 8 months ago (2013-07-21 02:21:51 UTC) #3
rharding
Please take a look.
10 years, 8 months ago (2013-07-22 11:29:33 UTC) #4
rharding
Updated to pull the charmID logic into the store method. It's probably the best thing ...
10 years, 8 months ago (2013-07-22 11:32:21 UTC) #5
gary.poster
On 2013/07/22 11:32:21, rharding wrote: > Updated to pull the charmID logic into the store ...
10 years, 8 months ago (2013-07-22 12:30:39 UTC) #6
benji
This looks good. The tests look quite good. LGTM-ly, y'rs Benji https://codereview.appspot.com/11499044/diff/8001/app/store/charm.js File app/store/charm.js (right): ...
10 years, 8 months ago (2013-07-22 13:00:06 UTC) #7
rharding
10 years, 8 months ago (2013-07-22 13:11:53 UTC) #8
*** Submitted:

Pull the default icon for local charms.

- Local charms do not exist on the back end api server so no icon is
available. The charm is 404'd.
- Local charms do not expose the category data so we cannot look for a
fallback icon.
- This branch just forces local charms to have the default charm icon via a
hard coded path in the store's charm icon url helper.

R=gary.poster, benji
CC=
https://codereview.appspot.com/11499044

https://codereview.appspot.com/11499044/diff/8001/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/11499044/diff/8001/app/store/charm.js#newcode366
app/store/charm.js:366: // colon portion of the quote and leaves behind a charm
ID.
On 2013/07/22 13:00:06, benji wrote:
> The comments in this function are nice.

Done.
Sign in to reply to this message.

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