juju-gui

Merge lp:~frankban/juju-gui/bug-1075672-icons into lp:juju-gui/experimental

  1. bug-1075672-icons
  2. Merge into trunk
Proposed by Francesco Banconi
Status: Merged
Merged at revision: 237
Proposed branch: lp:~frankban/juju-gui/bug-1075672-icons
Merge into: lp:juju-gui/experimental
Diff against target: 225 lines (+41/-21)
7 files modified
app/index.html (+1/-1)
app/templates/charm-description.handlebars (+5/-5)
app/templates/charm-pre-configuration.handlebars (+1/-1)
app/views/charm-panel.js (+29/-7)
lib/views/stylesheet.less (+2/-1)
test/test_charm_panel.js (+3/-3)
undocumented (+0/-3)
To merge this branch: bzr merge lp:~frankban/juju-gui/bug-1075672-icons
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+133944@code.launchpad.net

Description of the change

Replaced charm panel icons with our assets.

In the charm panel, replaced bootstrap icons with
the ones from our sprite. Also added the back triangle
to the sprite.
This branch also fixes the charm menu chevrons: now they
are correctly replaced on panel open/close.
Added "docstrings" for relevant methods.

https://codereview.appspot.com/6819131/

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :
Download full text (10.0 KiB)

Reviewers: mp+133944_code.launchpad.net,

Message:
Please take a look.

Description:
Replaced charm panel icons with our assets.

In the charm panel, replaced bootstrap icons with
the ones from our sprite. Also added the back triangle
to the sprite.
This branch also fixes the charm menu chevrons: now they
are correctly replaced on panel open/close.
Added "docstrings" for relevant methods.

https://code.launchpad.net/~frankban/juju-gui/bug-1075672-icons/+merge/133944

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6819131/

Affected files:
   A [revision details]
   A app/assets/images/back_triangle.png
   M app/index.html
   M app/templates/charm-description.handlebars
   M app/templates/charm-pre-configuration.handlebars
   M app/views/charm-panel.js
   M lib/views/stylesheet.less
   M test/test_charm_panel.js
   M undocumented

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision:
<email address hidden>

Index: app/index.html
=== modified file 'app/index.html'
--- app/index.html 2012-11-07 21:52:54 +0000
+++ app/index.html 2012-11-12 14:46:20 +0000
@@ -51,7 +51,7 @@
                      <span id="charm-search-trigger">
                        <i id="charm-search-icon" class="sprite
charm_icon"></i>
                        Charms
- <i class="icon-chevron-down"></i>
+ <i id="charm-search-chevron" class="sprite
chevron_down"></i>
                      </span>
                      <input type="text" id="charm-search-field"
                       required="required" placeholder="Search for a charm"
/>

Index: app/templates/charm-description.handlebars
=== modified file 'app/templates/charm-description.handlebars'
--- app/templates/charm-description.handlebars 2012-11-07 18:25:14 +0000
+++ app/templates/charm-description.handlebars 2012-11-12 14:46:20 +0000
@@ -1,5 +1,5 @@
  <div>
- <div class="charm-nav-back"><i class="icon-chevron-left"></i> Back</div>
+ <div class="charm-nav-back"><i class="sprite back_triangle"></i>
Back</div>

    <div class="charm-description charm-panel">
      <div id="charm-panel-head">
@@ -12,7 +12,7 @@
      </div>

      <div class="charm-section">
- <h4 class="first"><i class="icon-chevron-down"></i> Description</h4>
+ <h4 class="first"><i class="sprite chevron_up"></i> Description</h4>
        <div class="collapsible">
          <h5>Summary</h5>
          <p>{{summary}}</p>
@@ -33,7 +33,7 @@

      {{#any requires provides}}
      <div class="charm-section">
- <h4><i class="icon-chevron-up"></i> Interfaces</h4>
+ <h4><i class="sprite chevron_down"></i> Interfaces</h4>
        <div class="collapsible">
          {{#if provides}}
          <h5>Provides</h5>
@@ -57,7 +57,7 @@

      {{#if last_change}}
      <div class="charm-section">
- <h4><i class="icon-chevron-up"></i> Change Log</h4>
+ <h4><i class="sprite chevron_down"></i>...

Revision history for this message
Nicola Larosa (teknico) wrote :

LGTM, nothing to add.

https://codereview.appspot.com/6819131/

Revision history for this message
Brad Crittenden (bac) wrote :

Thanks Francesco. I'm concerned about being able to repeatedly open and
close the Charms panel using the top-level twisty.

Otherwise, looks good.

https://codereview.appspot.com/6819131/diff/1/app/templates/charm-description.handlebars
File app/templates/charm-description.handlebars (right):

https://codereview.appspot.com/6819131/diff/1/app/templates/charm-description.handlebars#newcode15
app/templates/charm-description.handlebars:15: <h4 class="first"><i
class="sprite chevron_up"></i> Description</h4>
I find chevron-up vs -down counter intuitive but it matches the visual
design.

Also I note with your branch that after loading I can click on the
Charms icon at the top and have it work exactly once. If I close it I
cannot reopen. Please check that behavior in your branch before
submitting.

Additionally it doesn't look like the chevron twists for the top Charms
label.

https://codereview.appspot.com/6819131/

Revision history for this message
Francesco Banconi (frankban) wrote :

On 2012/11/12 18:28:34, bac wrote:
> Thanks Francesco. I'm concerned about being able to repeatedly open
 and close
> the Charms panel using the top-level twisty.

> Otherwise, looks good.

Thanks for the review Brad.

https://codereview.appspot.com/6819131/diff/1/app/templates/charm-description.handlebars
> File app/templates/charm-description.handlebars (right):

https://codereview.appspot.com/6819131/diff/1/app/templates/charm-description.handlebars#newcode15
> app/templates/charm-description.handlebars:15: <h4 class="first"><i
> class="sprite chevron_up"></i> Description</h4>
> I find chevron-up vs -down counter intuitive but it matches the visual
 design.

Agreed.

> Also I note with your branch that after loading I can click on the
Charms icon
> at the top and have it work exactly once. If I close it I cannot
reopen.
> Please check that behavior in your branch before submitting.

Works for me locally, after cleaning the browser cache.

> Additionally it doesn't look like the chevron twists for the top
Charms label.

They are correctly replaced here. :-/ I think some kind of cache is
hitting us.

https://codereview.appspot.com/6819131/

Revision history for this message
Francesco Banconi (frankban) wrote :

*** Submitted:

Replaced charm panel icons with our assets.

In the charm panel, replaced bootstrap icons with
the ones from our sprite. Also added the back triangle
to the sprite.
This branch also fixes the charm menu chevrons: now they
are correctly replaced on panel open/close.
Added "docstrings" for relevant methods.

R=teknico, bac
CC=
https://codereview.appspot.com/6819131

https://codereview.appspot.com/6819131/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'app/assets/images/back_triangle.png'
2Binary files app/assets/images/back_triangle.png 1970-01-01 00:00:00 +0000 and app/assets/images/back_triangle.png 2012-11-12 15:24:20 +0000 differ
3=== modified file 'app/index.html'
4--- app/index.html 2012-11-07 21:52:54 +0000
5+++ app/index.html 2012-11-12 15:24:20 +0000
6@@ -51,7 +51,7 @@
7 <span id="charm-search-trigger">
8 <i id="charm-search-icon" class="sprite charm_icon"></i>
9 Charms
10- <i class="icon-chevron-down"></i>
11+ <i id="charm-search-chevron" class="sprite chevron_down"></i>
12 </span>
13 <input type="text" id="charm-search-field"
14 required="required" placeholder="Search for a charm" />
15
16=== modified file 'app/templates/charm-description.handlebars'
17--- app/templates/charm-description.handlebars 2012-11-07 18:25:14 +0000
18+++ app/templates/charm-description.handlebars 2012-11-12 15:24:20 +0000
19@@ -1,5 +1,5 @@
20 <div>
21- <div class="charm-nav-back"><i class="icon-chevron-left"></i> Back</div>
22+ <div class="charm-nav-back"><i class="sprite back_triangle"></i> Back</div>
23
24 <div class="charm-description charm-panel">
25 <div id="charm-panel-head">
26@@ -12,7 +12,7 @@
27 </div>
28
29 <div class="charm-section">
30- <h4 class="first"><i class="icon-chevron-down"></i> Description</h4>
31+ <h4 class="first"><i class="sprite chevron_up"></i> Description</h4>
32 <div class="collapsible">
33 <h5>Summary</h5>
34 <p>{{summary}}</p>
35@@ -33,7 +33,7 @@
36
37 {{#any requires provides}}
38 <div class="charm-section">
39- <h4><i class="icon-chevron-up"></i> Interfaces</h4>
40+ <h4><i class="sprite chevron_down"></i> Interfaces</h4>
41 <div class="collapsible">
42 {{#if provides}}
43 <h5>Provides</h5>
44@@ -57,7 +57,7 @@
45
46 {{#if last_change}}
47 <div class="charm-section">
48- <h4><i class="icon-chevron-up"></i> Change Log</h4>
49+ <h4><i class="sprite chevron_down"></i> Change Log</h4>
50 <div class="collapsible">
51 {{#with last_change}}
52 <h5>Last Change</h5>
53@@ -71,7 +71,7 @@
54
55 {{#any requires provides}}
56 <div class="charm-section">
57- <h4><i class="icon-chevron-up"></i> Related Charms</h4>
58+ <h4><i class="sprite chevron_down"></i> Related Charms</h4>
59 <div class="collapsible" id="related-charms">
60 Loading...
61 </div>
62
63=== modified file 'app/templates/charm-pre-configuration.handlebars'
64--- app/templates/charm-pre-configuration.handlebars 2012-11-09 11:45:00 +0000
65+++ app/templates/charm-pre-configuration.handlebars 2012-11-12 15:24:20 +0000
66@@ -43,7 +43,7 @@
67 {{#if settings}}
68 <!-- Service configuration form -->
69 <div class="charm-section">
70- <h4>Service Settings<i class="icon-chevron-down"></i></h4>
71+ <h4>Service Settings<i class="sprite chevron_up"></i></h4>
72 <div class="collapsible">
73 <div class="config-file-upload control-group">
74 <input class="config-file-upload-widget" type="file">
75
76=== modified file 'app/views/charm-panel.js'
77--- app/views/charm-panel.js 2012-11-09 20:51:13 +0000
78+++ app/views/charm-panel.js 2012-11-12 15:24:20 +0000
79@@ -39,10 +39,10 @@
80 // clientHeight and offsetHeight are not as reliable in tests.
81 if (parseInt(el.getStyle('height'), 10) === 0) {
82 el.show('sizeIn', {duration: 0.25, width: null});
83- icon.replaceClass('icon-chevron-up', 'icon-chevron-down');
84+ icon.replaceClass('chevron_down', 'chevron_up');
85 } else {
86 el.hide('sizeOut', {duration: 0.25, width: null});
87- icon.replaceClass('icon-chevron-down', 'icon-chevron-up');
88+ icon.replaceClass('chevron_up', 'chevron_down');
89 }
90 },
91 /**
92@@ -337,7 +337,7 @@
93 charm = this.get('model');
94 if (Y.Lang.isValue(charm)) {
95 container.setHTML(this.template(charm.getAttrs()));
96- container.all('i.icon-chevron-up').each(function(el) {
97+ container.all('i.chevron_down').each(function(el) {
98 el.ancestor('.charm-section').one('div')
99 .setStyle('height', '0px');
100 });
101@@ -881,6 +881,13 @@
102 }
103 });
104
105+ /**
106+ * Hide the charm panel.
107+ * Set isPanelVisible to false.
108+ *
109+ * @method hide
110+ * @return {undefined} Mutates only.
111+ */
112 function hide() {
113 if (isPanelVisible) {
114 var headerBox = Y.one('#charm-search-trigger-container'),
115@@ -893,8 +900,8 @@
116 }
117 container.hide(!testing, {duration: 0.25});
118 if (Y.Lang.isValue(trigger)) {
119- trigger.one('i').replaceClass(
120- 'icon-chevron-up', 'icon-chevron-down');
121+ trigger.one('i#charm-search-chevron').replaceClass(
122+ 'chevron_up', 'chevron_down');
123 }
124 isPanelVisible = false;
125 }
126@@ -912,6 +919,13 @@
127 }
128 }));
129
130+ /**
131+ * Show the charm panel.
132+ * Set isPanelVisible to true.
133+ *
134+ * @method show
135+ * @return {undefined} Mutates only.
136+ */
137 function show() {
138 if (!isPanelVisible) {
139 var headerBox = Y.one('#charm-search-trigger-container'),
140@@ -927,11 +941,19 @@
141 isPanelVisible = true;
142 updatePanelPosition();
143 if (Y.Lang.isValue(trigger)) {
144- trigger.one('i').replaceClass(
145- 'icon-chevron-down', 'icon-chevron-up');
146+ trigger.one('i#charm-search-chevron').replaceClass(
147+ 'chevron_down', 'chevron_up');
148 }
149 }
150 }
151+
152+ /**
153+ * Show the charm panel if it is hidden, hide it otherwise.
154+ *
155+ * @method toggle
156+ * @param {Object} ev An event object (with a "halt" method).
157+ * @return {undefined} Dispatches only.
158+ */
159 function toggle(ev) {
160 if (Y.Lang.isValue(ev)) {
161 // This is important to not have the clickoutside handler immediately
162
163=== modified file 'lib/views/stylesheet.less'
164--- lib/views/stylesheet.less 2012-11-09 14:30:56 +0000
165+++ lib/views/stylesheet.less 2012-11-12 15:24:20 +0000
166@@ -852,7 +852,7 @@
167 color: @charm-panel-deploy-button-color;
168 cursor: pointer;
169 font-size: 14px;
170- padding: 7px 4px;
171+ padding: 7px @charm-panel-padding-left;
172 }
173 .charm-panel {
174 background-color: @charm-panel-background-color;
175@@ -875,6 +875,7 @@
176 padding: 8px @charm-panel-padding-left;
177 i {
178 float: right;
179+ margin-top: 8px;
180 }
181 &.first {
182 border-top: 1px solid @charm-paneel-border-top-color;
183
184=== modified file 'test/test_charm_panel.js'
185--- test/test_charm_panel.js 2012-11-07 18:36:31 +0000
186+++ test/test_charm_panel.js 2012-11-12 15:24:20 +0000
187@@ -266,12 +266,12 @@
188 // We use the last change div.
189 section_container = html.one('div.charm-section:last-child');
190 section_container.one('div').getStyle('height').should.equal('0px');
191- assert(section_container.one('h4 i').hasClass('icon-chevron-up'));
192+ assert(section_container.one('h4 i').hasClass('chevron_down'));
193 section_container.one('h4').simulate('click');
194- assert(section_container.one('h4 i').hasClass('icon-chevron-down'));
195+ assert(section_container.one('h4 i').hasClass('chevron_up'));
196 section_container.one('div').getStyle('height').should.not.equal('0px');
197 section_container.one('h4').simulate('click');
198- assert(section_container.one('h4 i').hasClass('icon-chevron-up'));
199+ assert(section_container.one('h4 i').hasClass('chevron_down'));
200 // The transition is still running, so we can't check display.
201 });
202
203
204=== modified file 'undocumented'
205--- undocumented 2012-11-09 11:45:00 +0000
206+++ undocumented 2012-11-12 15:24:20 +0000
207@@ -68,7 +68,6 @@
208 app/views/charm-panel.js calculatePanelPosition
209 app/views/charm-panel.js createInstance
210 app/views/charm-panel.js getInstance
211-app/views/charm-panel.js hide
212 app/views/charm-panel.js hideDescription
213 app/views/charm-panel.js initializer
214 app/views/charm-panel.js killInstance
215@@ -79,10 +78,8 @@
216 app/views/charm-panel.js setDefaultSeries
217 app/views/charm-panel.js setPanel
218 app/views/charm-panel.js setupOverlay
219-app/views/charm-panel.js show
220 app/views/charm-panel.js showConfiguration
221 app/views/charm-panel.js showDescription
222-app/views/charm-panel.js toggle
223 app/views/charm-panel.js updatePanelPosition
224 app/views/charm.js _deployCallback
225 app/views/charm.js initializer

Subscribers

People subscribed via source and target branches