Merge lp:~frankban/juju-gui/bug-1075672-icons into lp:juju-gui/experimental
- bug-1075672-icons
- Merge into trunk
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju GUI Hackers | Pending | ||
Review via email: mp+133944@code.launchpad.net |
Commit message
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.
Francesco Banconi (frankban) wrote : | # |
Nicola Larosa (teknico) wrote : | # |
LGTM, nothing to add.
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:/
File app/templates/
https:/
app/templates/
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.
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:/
> File app/templates/
https:/
> app/templates/
> 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.
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:/
Preview Diff
1 | === added file 'app/assets/images/back_triangle.png' | |||
2 | 0 | Binary 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 | 0 | Binary 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 | 51 | <span id="charm-search-trigger"> | 51 | <span id="charm-search-trigger"> |
8 | 52 | <i id="charm-search-icon" class="sprite charm_icon"></i> | 52 | <i id="charm-search-icon" class="sprite charm_icon"></i> |
9 | 53 | Charms | 53 | Charms |
11 | 54 | <i class="icon-chevron-down"></i> | 54 | <i id="charm-search-chevron" class="sprite chevron_down"></i> |
12 | 55 | </span> | 55 | </span> |
13 | 56 | <input type="text" id="charm-search-field" | 56 | <input type="text" id="charm-search-field" |
14 | 57 | required="required" placeholder="Search for a charm" /> | 57 | required="required" placeholder="Search for a charm" /> |
15 | 58 | 58 | ||
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 | 1 | <div> | 1 | <div> |
22 | 2 | <div class="charm-nav-back"><i class="icon-chevron-left"></i> Back</div> | 2 | <div class="charm-nav-back"><i class="sprite back_triangle"></i> Back</div> |
23 | 3 | 3 | ||
24 | 4 | <div class="charm-description charm-panel"> | 4 | <div class="charm-description charm-panel"> |
25 | 5 | <div id="charm-panel-head"> | 5 | <div id="charm-panel-head"> |
26 | @@ -12,7 +12,7 @@ | |||
27 | 12 | </div> | 12 | </div> |
28 | 13 | 13 | ||
29 | 14 | <div class="charm-section"> | 14 | <div class="charm-section"> |
31 | 15 | <h4 class="first"><i class="icon-chevron-down"></i> Description</h4> | 15 | <h4 class="first"><i class="sprite chevron_up"></i> Description</h4> |
32 | 16 | <div class="collapsible"> | 16 | <div class="collapsible"> |
33 | 17 | <h5>Summary</h5> | 17 | <h5>Summary</h5> |
34 | 18 | <p>{{summary}}</p> | 18 | <p>{{summary}}</p> |
35 | @@ -33,7 +33,7 @@ | |||
36 | 33 | 33 | ||
37 | 34 | {{#any requires provides}} | 34 | {{#any requires provides}} |
38 | 35 | <div class="charm-section"> | 35 | <div class="charm-section"> |
40 | 36 | <h4><i class="icon-chevron-up"></i> Interfaces</h4> | 36 | <h4><i class="sprite chevron_down"></i> Interfaces</h4> |
41 | 37 | <div class="collapsible"> | 37 | <div class="collapsible"> |
42 | 38 | {{#if provides}} | 38 | {{#if provides}} |
43 | 39 | <h5>Provides</h5> | 39 | <h5>Provides</h5> |
44 | @@ -57,7 +57,7 @@ | |||
45 | 57 | 57 | ||
46 | 58 | {{#if last_change}} | 58 | {{#if last_change}} |
47 | 59 | <div class="charm-section"> | 59 | <div class="charm-section"> |
49 | 60 | <h4><i class="icon-chevron-up"></i> Change Log</h4> | 60 | <h4><i class="sprite chevron_down"></i> Change Log</h4> |
50 | 61 | <div class="collapsible"> | 61 | <div class="collapsible"> |
51 | 62 | {{#with last_change}} | 62 | {{#with last_change}} |
52 | 63 | <h5>Last Change</h5> | 63 | <h5>Last Change</h5> |
53 | @@ -71,7 +71,7 @@ | |||
54 | 71 | 71 | ||
55 | 72 | {{#any requires provides}} | 72 | {{#any requires provides}} |
56 | 73 | <div class="charm-section"> | 73 | <div class="charm-section"> |
58 | 74 | <h4><i class="icon-chevron-up"></i> Related Charms</h4> | 74 | <h4><i class="sprite chevron_down"></i> Related Charms</h4> |
59 | 75 | <div class="collapsible" id="related-charms"> | 75 | <div class="collapsible" id="related-charms"> |
60 | 76 | Loading... | 76 | Loading... |
61 | 77 | </div> | 77 | </div> |
62 | 78 | 78 | ||
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 | 43 | {{#if settings}} | 43 | {{#if settings}} |
68 | 44 | <!-- Service configuration form --> | 44 | <!-- Service configuration form --> |
69 | 45 | <div class="charm-section"> | 45 | <div class="charm-section"> |
71 | 46 | <h4>Service Settings<i class="icon-chevron-down"></i></h4> | 46 | <h4>Service Settings<i class="sprite chevron_up"></i></h4> |
72 | 47 | <div class="collapsible"> | 47 | <div class="collapsible"> |
73 | 48 | <div class="config-file-upload control-group"> | 48 | <div class="config-file-upload control-group"> |
74 | 49 | <input class="config-file-upload-widget" type="file"> | 49 | <input class="config-file-upload-widget" type="file"> |
75 | 50 | 50 | ||
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 | 39 | // clientHeight and offsetHeight are not as reliable in tests. | 39 | // clientHeight and offsetHeight are not as reliable in tests. |
81 | 40 | if (parseInt(el.getStyle('height'), 10) === 0) { | 40 | if (parseInt(el.getStyle('height'), 10) === 0) { |
82 | 41 | el.show('sizeIn', {duration: 0.25, width: null}); | 41 | el.show('sizeIn', {duration: 0.25, width: null}); |
84 | 42 | icon.replaceClass('icon-chevron-up', 'icon-chevron-down'); | 42 | icon.replaceClass('chevron_down', 'chevron_up'); |
85 | 43 | } else { | 43 | } else { |
86 | 44 | el.hide('sizeOut', {duration: 0.25, width: null}); | 44 | el.hide('sizeOut', {duration: 0.25, width: null}); |
88 | 45 | icon.replaceClass('icon-chevron-down', 'icon-chevron-up'); | 45 | icon.replaceClass('chevron_up', 'chevron_down'); |
89 | 46 | } | 46 | } |
90 | 47 | }, | 47 | }, |
91 | 48 | /** | 48 | /** |
92 | @@ -337,7 +337,7 @@ | |||
93 | 337 | charm = this.get('model'); | 337 | charm = this.get('model'); |
94 | 338 | if (Y.Lang.isValue(charm)) { | 338 | if (Y.Lang.isValue(charm)) { |
95 | 339 | container.setHTML(this.template(charm.getAttrs())); | 339 | container.setHTML(this.template(charm.getAttrs())); |
97 | 340 | container.all('i.icon-chevron-up').each(function(el) { | 340 | container.all('i.chevron_down').each(function(el) { |
98 | 341 | el.ancestor('.charm-section').one('div') | 341 | el.ancestor('.charm-section').one('div') |
99 | 342 | .setStyle('height', '0px'); | 342 | .setStyle('height', '0px'); |
100 | 343 | }); | 343 | }); |
101 | @@ -881,6 +881,13 @@ | |||
102 | 881 | } | 881 | } |
103 | 882 | }); | 882 | }); |
104 | 883 | 883 | ||
105 | 884 | /** | ||
106 | 885 | * Hide the charm panel. | ||
107 | 886 | * Set isPanelVisible to false. | ||
108 | 887 | * | ||
109 | 888 | * @method hide | ||
110 | 889 | * @return {undefined} Mutates only. | ||
111 | 890 | */ | ||
112 | 884 | function hide() { | 891 | function hide() { |
113 | 885 | if (isPanelVisible) { | 892 | if (isPanelVisible) { |
114 | 886 | var headerBox = Y.one('#charm-search-trigger-container'), | 893 | var headerBox = Y.one('#charm-search-trigger-container'), |
115 | @@ -893,8 +900,8 @@ | |||
116 | 893 | } | 900 | } |
117 | 894 | container.hide(!testing, {duration: 0.25}); | 901 | container.hide(!testing, {duration: 0.25}); |
118 | 895 | if (Y.Lang.isValue(trigger)) { | 902 | if (Y.Lang.isValue(trigger)) { |
121 | 896 | trigger.one('i').replaceClass( | 903 | trigger.one('i#charm-search-chevron').replaceClass( |
122 | 897 | 'icon-chevron-up', 'icon-chevron-down'); | 904 | 'chevron_up', 'chevron_down'); |
123 | 898 | } | 905 | } |
124 | 899 | isPanelVisible = false; | 906 | isPanelVisible = false; |
125 | 900 | } | 907 | } |
126 | @@ -912,6 +919,13 @@ | |||
127 | 912 | } | 919 | } |
128 | 913 | })); | 920 | })); |
129 | 914 | 921 | ||
130 | 922 | /** | ||
131 | 923 | * Show the charm panel. | ||
132 | 924 | * Set isPanelVisible to true. | ||
133 | 925 | * | ||
134 | 926 | * @method show | ||
135 | 927 | * @return {undefined} Mutates only. | ||
136 | 928 | */ | ||
137 | 915 | function show() { | 929 | function show() { |
138 | 916 | if (!isPanelVisible) { | 930 | if (!isPanelVisible) { |
139 | 917 | var headerBox = Y.one('#charm-search-trigger-container'), | 931 | var headerBox = Y.one('#charm-search-trigger-container'), |
140 | @@ -927,11 +941,19 @@ | |||
141 | 927 | isPanelVisible = true; | 941 | isPanelVisible = true; |
142 | 928 | updatePanelPosition(); | 942 | updatePanelPosition(); |
143 | 929 | if (Y.Lang.isValue(trigger)) { | 943 | if (Y.Lang.isValue(trigger)) { |
146 | 930 | trigger.one('i').replaceClass( | 944 | trigger.one('i#charm-search-chevron').replaceClass( |
147 | 931 | 'icon-chevron-down', 'icon-chevron-up'); | 945 | 'chevron_down', 'chevron_up'); |
148 | 932 | } | 946 | } |
149 | 933 | } | 947 | } |
150 | 934 | } | 948 | } |
151 | 949 | |||
152 | 950 | /** | ||
153 | 951 | * Show the charm panel if it is hidden, hide it otherwise. | ||
154 | 952 | * | ||
155 | 953 | * @method toggle | ||
156 | 954 | * @param {Object} ev An event object (with a "halt" method). | ||
157 | 955 | * @return {undefined} Dispatches only. | ||
158 | 956 | */ | ||
159 | 935 | function toggle(ev) { | 957 | function toggle(ev) { |
160 | 936 | if (Y.Lang.isValue(ev)) { | 958 | if (Y.Lang.isValue(ev)) { |
161 | 937 | // This is important to not have the clickoutside handler immediately | 959 | // This is important to not have the clickoutside handler immediately |
162 | 938 | 960 | ||
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 | 852 | color: @charm-panel-deploy-button-color; | 852 | color: @charm-panel-deploy-button-color; |
168 | 853 | cursor: pointer; | 853 | cursor: pointer; |
169 | 854 | font-size: 14px; | 854 | font-size: 14px; |
171 | 855 | padding: 7px 4px; | 855 | padding: 7px @charm-panel-padding-left; |
172 | 856 | } | 856 | } |
173 | 857 | .charm-panel { | 857 | .charm-panel { |
174 | 858 | background-color: @charm-panel-background-color; | 858 | background-color: @charm-panel-background-color; |
175 | @@ -875,6 +875,7 @@ | |||
176 | 875 | padding: 8px @charm-panel-padding-left; | 875 | padding: 8px @charm-panel-padding-left; |
177 | 876 | i { | 876 | i { |
178 | 877 | float: right; | 877 | float: right; |
179 | 878 | margin-top: 8px; | ||
180 | 878 | } | 879 | } |
181 | 879 | &.first { | 880 | &.first { |
182 | 880 | border-top: 1px solid @charm-paneel-border-top-color; | 881 | border-top: 1px solid @charm-paneel-border-top-color; |
183 | 881 | 882 | ||
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 | 266 | // We use the last change div. | 266 | // We use the last change div. |
189 | 267 | section_container = html.one('div.charm-section:last-child'); | 267 | section_container = html.one('div.charm-section:last-child'); |
190 | 268 | section_container.one('div').getStyle('height').should.equal('0px'); | 268 | section_container.one('div').getStyle('height').should.equal('0px'); |
192 | 269 | assert(section_container.one('h4 i').hasClass('icon-chevron-up')); | 269 | assert(section_container.one('h4 i').hasClass('chevron_down')); |
193 | 270 | section_container.one('h4').simulate('click'); | 270 | section_container.one('h4').simulate('click'); |
195 | 271 | assert(section_container.one('h4 i').hasClass('icon-chevron-down')); | 271 | assert(section_container.one('h4 i').hasClass('chevron_up')); |
196 | 272 | section_container.one('div').getStyle('height').should.not.equal('0px'); | 272 | section_container.one('div').getStyle('height').should.not.equal('0px'); |
197 | 273 | section_container.one('h4').simulate('click'); | 273 | section_container.one('h4').simulate('click'); |
199 | 274 | assert(section_container.one('h4 i').hasClass('icon-chevron-up')); | 274 | assert(section_container.one('h4 i').hasClass('chevron_down')); |
200 | 275 | // The transition is still running, so we can't check display. | 275 | // The transition is still running, so we can't check display. |
201 | 276 | }); | 276 | }); |
202 | 277 | 277 | ||
203 | 278 | 278 | ||
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 | 68 | app/views/charm-panel.js calculatePanelPosition | 68 | app/views/charm-panel.js calculatePanelPosition |
209 | 69 | app/views/charm-panel.js createInstance | 69 | app/views/charm-panel.js createInstance |
210 | 70 | app/views/charm-panel.js getInstance | 70 | app/views/charm-panel.js getInstance |
211 | 71 | app/views/charm-panel.js hide | ||
212 | 72 | app/views/charm-panel.js hideDescription | 71 | app/views/charm-panel.js hideDescription |
213 | 73 | app/views/charm-panel.js initializer | 72 | app/views/charm-panel.js initializer |
214 | 74 | app/views/charm-panel.js killInstance | 73 | app/views/charm-panel.js killInstance |
215 | @@ -79,10 +78,8 @@ | |||
216 | 79 | app/views/charm-panel.js setDefaultSeries | 78 | app/views/charm-panel.js setDefaultSeries |
217 | 80 | app/views/charm-panel.js setPanel | 79 | app/views/charm-panel.js setPanel |
218 | 81 | app/views/charm-panel.js setupOverlay | 80 | app/views/charm-panel.js setupOverlay |
219 | 82 | app/views/charm-panel.js show | ||
220 | 83 | app/views/charm-panel.js showConfiguration | 81 | app/views/charm-panel.js showConfiguration |
221 | 84 | app/views/charm-panel.js showDescription | 82 | app/views/charm-panel.js showDescription |
222 | 85 | app/views/charm-panel.js toggle | ||
223 | 86 | app/views/charm-panel.js updatePanelPosition | 83 | app/views/charm-panel.js updatePanelPosition |
224 | 87 | app/views/charm.js _deployCallback | 84 | app/views/charm.js _deployCallback |
225 | 88 | app/views/charm.js initializer | 85 | app/views/charm.js initializer |
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: images/ back_triangle. png charm-descripti on.handlebars charm-pre- configuration. handlebars charm-panel. js stylesheet. less charm_panel. js
A [revision details]
A app/assets/
M app/index.html
M app/templates/
M app/templates/
M app/views/
M lib/views/
M test/test_
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
<span id="charm- search- trigger" >
<i id="charm- search- icon" class="sprite
Charms icon-chevron- down">< /i> search- chevron" class="sprite
</span>
<input type="text" id="charm- search- field"
required= "required" placeholder="Search for a charm"
=== 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 @@
charm_icon"></i>
- <i class="
+ <i id="charm-
chevron_down"></i>
/>
Index: app/templates/ charm-descripti on.handlebars charm-descripti on.handlebars' charm-descripti on.handlebars 2012-11-07 18:25:14 +0000 charm-descripti on.handlebars 2012-11-12 14:46:20 +0000 charm-nav- back">< i class=" icon-chevron- left">< /i> Back</div> charm-nav- back">< i class="sprite back_triangle"></i>
=== modified file 'app/templates/
--- app/templates/
+++ app/templates/
@@ -1,5 +1,5 @@
<div>
- <div class="
+ <div class="
Back</div>
<div class=" charm-descripti on charm-panel"> panel-head" >
<div id="charm-
@@ -12,7 +12,7 @@
</div>
<div class=" charm-section" > icon-chevron- down">< /i> Description</h4> collapsible" >
<h5> Summary< /h5>
<p>{ {summary} }</p>
- <h4 class="first"><i class="
+ <h4 class="first"><i class="sprite chevron_up"></i> Description</h4>
<div class="
@@ -33,7 +33,7 @@
{{#any requires provides}} charm-section" > icon-chevron- up"></i> Interfaces</h4> collapsible" >
<h5> Provides< /h5>
<div class="
- <h4><i class="
+ <h4><i class="sprite chevron_down"></i> Interfaces</h4>
<div class="
{{#if provides}}
@@ -57,7 +57,7 @@
{{#if last_change}} charm-section" > icon-chevron- up"></i> Change Log</h4> down">< /i>...
<div class="
- <h4><i class="
+ <h4><i class="sprite chevron_