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}
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
5 years, 9 months ago
(2015-03-03 00:19:05 UTC)
#11
Patchset #4 (id:60001) has been deleted
stevenjb
Patchset #4 (id:80001) has been deleted
5 years, 9 months ago
(2015-03-03 00:26:32 UTC)
#12
Patchset #4 (id:80001) has been deleted
stevenjb
Patchset #4 (id:100001) has been deleted
5 years, 9 months ago
(2015-03-03 00:29:39 UTC)
#13
Patchset #4 (id:100001) has been deleted
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
OK, I made some changes to use Expressions instead of setting 'hidden' and 'src'
directly. I think that is probably a better precedent to set.
I did not use Expressions for setting the class list, because the logic is more
complex and depends on multiple inputs; the result seemed more convoluted.
PTAL
https://codereview.chromium.org/954293003/diff/40001/ui/webui/resources/cr_el...
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_el...
ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:61:
On 2015/02/28 00:53:32, Jeremy Klein wrote:
> On 2015/02/28 00:17:06, michaelpg wrote:
> > nit.. "right"
>
> Also "Theese" :-)
Done.
https://codereview.chromium.org/954293003/diff/40001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.css:65:
margin-left: 0px;
On 2015/02/28 00:53:32, Jeremy Klein wrote:
> For position: absolute, we should just be using left, right, bottom, and top
> instead of margins.
Done.
https://codereview.chromium.org/954293003/diff/40001/ui/webui/resources/cr_el...
File ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js (right):
https://codereview.chromium.org/954293003/diff/40001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:129: setIcon_:
function(params) {
On 2015/02/28 00:53:32, Jeremy Klein wrote:
> nit: This function is pretty big. Do you mind breaking it up a bit?
Done.
https://codereview.chromium.org/954293003/diff/40001/ui/webui/resources/cr_el...
ui/webui/resources/cr_elements/cr_network_icon/cr_network_icon.js:203:
this.$.technology.hidden = !technology;
On 2015/02/28 00:53:32, Jeremy Klein wrote:
> Totally optional, but all this badge logic could be bound using computed
> properties:
>
> https://www.polymer-project.org/docs/polymer/polymer.html#computed-properties
>
> for "technology" and "secure" here and then doing something like:
>
> <img id="technology src="{{technologyImgSrc}}" hidden?="{{!technology}}>
>
> etc.
>
> What you've done here is totally fine too, just presenting an option.
I explored this and decided to use expressions for the src and hidden properties
(which are straightforward), but not for the class visibility, which depends on
mulitple inputs and would end up triggering multiple change events per call to
setIcon_().
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
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
The patchset sent to the CQ was uploaded after l-g-t-m from jhawkins@chromium.org, pneubeck@chromium.org, michaelpg@chromium.org, jlklein@chromium.org ...
5 years, 9 months ago
(2015-03-03 01:08:11 UTC)
#18
5 years, 9 months ago
(2015-03-03 01:26:20 UTC)
#21
https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right):
https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:31: connected: function() {
On 2015/03/03 01:07:08, stevenjb wrote:
> On 2015/03/03 00:41:08, Jeremy Klein wrote:
> > Another totally optional alternative polymer tip:
> >
> > You can make this and connecting computed properties:
> >
> >
https://www.polymer-project.org/docs/polymer/polymer.html#computed-properties
> >
> > so that you could bind directly to them in clients if you want.
>
> Interesting thought. When looking at Expressions for cr-network-icon, I
decided
> I wasn't a huge fan of the computed {} section in Polymer, since it is
embedding
> logic in a string. Using Expressions in HTML makes sense to me, but here I
think
> I prefer straight JS. We'll see, maybe I will warm up to the idea.
I see what you mean about the string, but computed properties are entirely in
JS, not in the HTML. So this would look like:
computed: {
connected: 'data.ConnectionState == Cr.Onc.ConnectionState.CONNECTED',
connecting: 'data.ConnectionState == Cr.Onc.ConnectionState.CONNECTING',
}
and then you'd include these properties in the publish block if you want and you
could bind to them in clients:
<cr-network-icon class="{{ {connecting: data.connected, connected:
data.connecting} | tokenList}}"></cr-network-icon>
Just a thought. Not super important.
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
On 2015/03/03 01:26:20, Jeremy Klein wrote:
>
https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_e...
> File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right):
>
>
https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_e...
> ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:31: connected: function()
{
> On 2015/03/03 01:07:08, stevenjb wrote:
> > On 2015/03/03 00:41:08, Jeremy Klein wrote:
> > > Another totally optional alternative polymer tip:
> > >
> > > You can make this and connecting computed properties:
> > >
> > >
> https://www.polymer-project.org/docs/polymer/polymer.html#computed-properties
> > >
> > > so that you could bind directly to them in clients if you want.
> >
> > Interesting thought. When looking at Expressions for cr-network-icon, I
> decided
> > I wasn't a huge fan of the computed {} section in Polymer, since it is
> embedding
> > logic in a string. Using Expressions in HTML makes sense to me, but here I
> think
> > I prefer straight JS. We'll see, maybe I will warm up to the idea.
>
> I see what you mean about the string, but computed properties are entirely in
> JS, not in the HTML. So this would look like:
>
> computed: {
> connected: 'data.ConnectionState == Cr.Onc.ConnectionState.CONNECTED',
> connecting: 'data.ConnectionState == Cr.Onc.ConnectionState.CONNECTING',
> }
>
> and then you'd include these properties in the publish block if you want and
you
> could bind to them in clients:
>
> <cr-network-icon class="{{ {connecting: data.connected, connected:
> data.connecting} | tokenList}}"></cr-network-icon>
>
> Just a thought. Not super important.
Agreed it's not critical here, but it's a good discussion point I think for the
style that we are developing.
I do see the advantages to your example, the thing I don't like is that the JS
line:
connected: 'data.ConnectionState == Cr.Onc.ConnectionState.CONNECTED',
Since the JS is encoded in a string, it looks "wrong" in my editor, and I don't
imagine it would benefit from any Closure checks either.
It gets even stranger if, e.g. we weren't using constants:
connected: 'data.ConnectionState == \'Connected\'',
We can avoid that by creating custom filters (which is what I did for
cr-network-icon), e.g:
connected: 'data | isConnected',
Which I could warm up to, but I actually find the example you gave for
describing a <cr-network-icon> a little challenging to parse and I wrote the
code :)
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
On 2015/03/03 01:48:58, stevenjb wrote:
> On 2015/03/03 01:26:20, Jeremy Klein wrote:
> >
>
https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_e...
> > File ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js (right):
> >
> >
>
https://codereview.chromium.org/954293003/diff/120001/ui/webui/resources/cr_e...
> > ui/webui/resources/cr_elements/cr_onc/cr_onc_data.js:31: connected:
function()
> {
> > On 2015/03/03 01:07:08, stevenjb wrote:
> > > On 2015/03/03 00:41:08, Jeremy Klein wrote:
> > > > Another totally optional alternative polymer tip:
> > > >
> > > > You can make this and connecting computed properties:
> > > >
> > > >
> >
https://www.polymer-project.org/docs/polymer/polymer.html#computed-properties
> > > >
> > > > so that you could bind directly to them in clients if you want.
> > >
> > > Interesting thought. When looking at Expressions for cr-network-icon, I
> > decided
> > > I wasn't a huge fan of the computed {} section in Polymer, since it is
> > embedding
> > > logic in a string. Using Expressions in HTML makes sense to me, but here I
> > think
> > > I prefer straight JS. We'll see, maybe I will warm up to the idea.
> >
> > I see what you mean about the string, but computed properties are entirely
in
> > JS, not in the HTML. So this would look like:
> >
> > computed: {
> > connected: 'data.ConnectionState == Cr.Onc.ConnectionState.CONNECTED',
> > connecting: 'data.ConnectionState == Cr.Onc.ConnectionState.CONNECTING',
> > }
> >
> > and then you'd include these properties in the publish block if you want and
> you
> > could bind to them in clients:
> >
> > <cr-network-icon class="{{ {connecting: data.connected, connected:
> > data.connecting} | tokenList}}"></cr-network-icon>
> >
> > Just a thought. Not super important.
>
> Agreed it's not critical here, but it's a good discussion point I think for
the
> style that we are developing.
>
> I do see the advantages to your example, the thing I don't like is that the JS
> line:
>
> connected: 'data.ConnectionState == Cr.Onc.ConnectionState.CONNECTED',
>
> Since the JS is encoded in a string, it looks "wrong" in my editor, and I
don't
> imagine it would benefit from any Closure checks either.
>
> It gets even stranger if, e.g. we weren't using constants:
>
> connected: 'data.ConnectionState == \'Connected\'',
>
> We can avoid that by creating custom filters (which is what I did for
> cr-network-icon), e.g:
>
> connected: 'data | isConnected',
>
> Which I could warm up to, but I actually find the example you gave for
> describing a <cr-network-icon> a little challenging to parse and I wrote the
> code :)
Fair enough. There's also the option of using functions in the computed strings
to get a bit better type checking (connected: 'computeConnected()'). I'm not
sure of the side effects of that, but it's worth exploring. Either way, this
doesn't seem to be a major win at the moment.
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
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;
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).
commit-bot: I haz the power
Committed patchset #5 (id:140001)
5 years, 9 months ago
(2015-03-03 03:08:33 UTC)
#25
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
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
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; On 2015/03/03 02:52:04, michaelpg wrote: > I ...
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.
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
Reviewers: pneubeck (no reviews), michaelpg, Jeremy Klein, James Hawkins, Dan Beam, Kyle Horimoto, Oren Blasberg
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 37