Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(153)

Issue 954293003: Support badges and connecting animation in cr-network-icon element (Closed)

Created:
5 years, 10 months ago by stevenjb
Modified:
5 years, 9 months ago
CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, nkostylev+watch_chromium.org, hashimoto+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, jlklein+watch-md-settings_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Support badges and connecting animation in cr-network-icon element This is a follow-up to https://codereview.chromium.org/874283006 which introduces the cr-network-icon Polymer element for displaying an icon representing network state in web UI. It also includes minor changes to the fake shill implementation to facilitate manual testing. BUG=455499 Committed: https://crrev.com/6014e9a164d3f17b96027dd9c090b06ce9614f7d Cr-Commit-Position: refs/heads/master@{#318824}

Patch Set 1 #

Patch Set 2 : . #

Total comments: 21

Patch Set 3 : Feedback #

Total comments: 9

Patch Set 4 : Use computed properties #

Total comments: 7

Patch Set 5 : More Feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -60 lines) Patch
M chrome/browser/resources/chromeos/network_ui/network_ui.css View 1 chunk +5 lines, -0 lines 0 comments Download
M chromeos/chromeos_switches.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M chromeos/dbus/fake_shill_manager_client.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M chromeos/dbus/fake_shill_manager_client.cc View 1 2 5 chunks +25 lines, -4 lines 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_3g.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_4g.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_edge.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_evdo.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_gsm.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_hspa.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_hspa_plus.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_lte.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_lte_advanced.png View Binary file 0 comments Download
A ui/webui/resources/cr_elements/cr_network_icon/badge_roaming.png View Binary file 0 comments Download
A + ui/webui/resources/cr_elements/cr_network_icon/badge_secure.png View Binary file 0 comments Download
M ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css View 1 2 3 2 chunks +33 lines, -2 lines 0 comments Download
M ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html View 1 2 3 1 chunk +8 lines, -2 lines 0 comments Download
M ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js View 1 2 3 4 6 chunks +136 lines, -26 lines 0 comments Download
M ui/webui/resources/cr_elements/cr_network_icon/mobile.png View Binary file 0 comments Download
D ui/webui/resources/cr_elements/cr_network_icon/secure.png View Binary file 0 comments Download
M ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js View 1 2 chunks +26 lines, -8 lines 0 comments Download
M ui/webui/resources/cr_elements/cr_onc/cr_onc_types.js View 2 chunks +55 lines, -13 lines 0 comments Download
M ui/webui/resources/cr_elements_images.grdp View 1 chunk +35 lines, -5 lines 0 comments Download

Messages

Total messages: 27 (7 generated)
stevenjb
pneubeck@ - PTAL chromeos/ changes, plus FYI
5 years, 10 months ago (2015-02-26 00:07:25 UTC) #2
pneubeck (no reviews)
chromeos/ lgtm https://codereview.chromium.org/954293003/diff/20001/chromeos/chromeos_switches.cc File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/954293003/diff/20001/chromeos/chromeos_switches.cc#newcode274 chromeos/chromeos_switches.cc:274: // 'cellular=LTE' - Cellular is initially connected, ...
5 years, 10 months ago (2015-02-26 09:11:15 UTC) #3
stevenjb
Ping (anyone from the settings team)
5 years, 9 months ago (2015-02-27 19:26:50 UTC) #5
James Hawkins
LGTM with nits. https://codereview.chromium.org/954293003/diff/20001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): https://codereview.chromium.org/954293003/diff/20001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css#newcode64 ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:64: margin-left: 0px; -webkit-margin-start https://codereview.chromium.org/954293003/diff/20001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css#newcode73 ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:73: margin-left: ...
5 years, 9 months ago (2015-02-27 19:46:05 UTC) #6
michaelpg
https://codereview.chromium.org/954293003/diff/20001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html (right): https://codereview.chromium.org/954293003/diff/20001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html#newcode9 ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.html:9: src="chrome://resources/cr_elements/cr_network_icon/badge_roaming.png"> I wonder if it would be better not ...
5 years, 9 months ago (2015-02-27 20:38:41 UTC) #7
stevenjb
https://codereview.chromium.org/954293003/diff/20001/chromeos/chromeos_switches.cc File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/954293003/diff/20001/chromeos/chromeos_switches.cc#newcode274 chromeos/chromeos_switches.cc:274: // 'cellular=LTE' - Cellular is initially connected, technology is ...
5 years, 9 months ago (2015-02-27 22:14:06 UTC) #8
michaelpg
lgtm https://codereview.chromium.org/954293003/diff/20001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/954293003/diff/20001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js#newcode194 ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:194: this.$.technology.hidden = !technology; On 2015/02/27 22:14:06, stevenjb wrote: ...
5 years, 9 months ago (2015-02-28 00:17:06 UTC) #9
Jeremy Klein
lgtm Cool stuff! https://codereview.chromium.org/954293003/diff/40001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css (right): https://codereview.chromium.org/954293003/diff/40001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css#newcode61 ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:61: On 2015/02/28 00:17:06, michaelpg wrote: > ...
5 years, 9 months ago (2015-02-28 00:53:32 UTC) #10
stevenjb
OK, I made some changes to use Expressions instead of setting 'hidden' and 'src' directly. ...
5 years, 9 months ago (2015-03-03 00:32:03 UTC) #14
Jeremy Klein
lgtm Very cool to see some of this more advanced Polymer stuff in action in ...
5 years, 9 months ago (2015-03-03 00:41:08 UTC) #15
stevenjb
https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right): https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js#newcode52 ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:52: // The icon type to use for the base ...
5 years, 9 months ago (2015-03-03 01:07:08 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/954293003/140001
5 years, 9 months ago (2015-03-03 01:08:24 UTC) #19
stevenjb
(Committing this, I'll address any further nits in the next CL)
5 years, 9 months ago (2015-03-03 01:08:35 UTC) #20
Jeremy Klein
https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right): https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js#newcode31 ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:31: connected: function() { On 2015/03/03 01:07:08, stevenjb wrote: > ...
5 years, 9 months ago (2015-03-03 01:26:20 UTC) #21
stevenjb
On 2015/03/03 01:26:20, Jeremy Klein wrote: > https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js > File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right): > > https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js#newcode31 ...
5 years, 9 months ago (2015-03-03 01:48:58 UTC) #22
Jeremy Klein
On 2015/03/03 01:48:58, stevenjb wrote: > On 2015/03/03 01:26:20, Jeremy Klein wrote: > > > ...
5 years, 9 months ago (2015-03-03 01:56:05 UTC) #23
michaelpg
https://codereview.chromium.org/954293003/diff/20001/chrome/browser/resources/chromeos/network_ui/network_ui.css File chrome/browser/resources/chromeos/network_ui/network_ui.css (right): https://codereview.chromium.org/954293003/diff/20001/chrome/browser/resources/chromeos/network_ui/network_ui.css#newcode54 chrome/browser/resources/chromeos/network_ui/network_ui.css:54: display: flex; I don't know why exactly but this ...
5 years, 9 months ago (2015-03-03 02:52:04 UTC) #24
commit-bot: I haz the power
Committed patchset #5 (id:140001)
5 years, 9 months ago (2015-03-03 03:08:33 UTC) #25
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/6014e9a164d3f17b96027dd9c090b06ce9614f7d Cr-Commit-Position: refs/heads/master@{#318824}
5 years, 9 months ago (2015-03-03 03:09:36 UTC) #26
stevenjb
5 years, 9 months ago (2015-03-03 17:55:28 UTC) #27
Message was sent while issue was closed.
https://codereview.chromium.org/954293003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/network_ui/network_ui.css (right):

https://codereview.chromium.org/954293003/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/network_ui/network_ui.css:54: display: flex;
On 2015/03/03 02:52:04, michaelpg wrote:
> I don't know why exactly but this affects the flow somehow, adding a second
> border around the icons:
> 
> http://i.imgur.com/r5YH1eh.png
> 
> I disabled "display:flex" for the 3rd icon (cellular) -- see that it has the
> regular border width.
> 
> I also set "display: flex" on the "Type" cell in the first row (eth1_gui) --
as
> you can see the border surrounds the text content, not the entire <td> (or
> rather, it seems the <td> is  sized around the text content, making it smaller
> than the space it should occupy in the table).

Good catch. It seems that display: flex does not play well with tables (or at
least, after a bit of playing it is still not clear to me wtf is going on).

Adding .state-table cr-network-icon { vertical-align: middle; } seems to be the
correct thing to do here. I'll put up a CL with that shortly.

Powered by Google App Engine
This is Rietveld 408576698