[MD Extensions] Update Item Card UI

[MD Extensions] Update Item Card UI

We have new mocks!  Update the item card UI to be more in-line with the
new specs.

BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2707083003
Cr-Commit-Position: refs/heads/master@{#453297}
Committed: https://chromium.googlesource.com/chromium/src/+/4c12e06b9f81f8845fa504213ebc3077773ca444

[Devlin, 2017-02-22 01:43:08]

Description was changed from

==========
Item refresh
BUG=
==========

to

==========
Item refresh
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Devlin, 2017-02-22 17:24:37]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 17:25:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 18:04:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 18:04:03]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Devlin, 2017-02-22 19:43:38]

Description was changed from

==========
Item refresh
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Extensions] Update Item Card UI

We have new mocks!  Update the item card UI to be more in-line with the
new specs.

BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Devlin, 2017-02-22 19:50:25]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 19:51:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 21:30:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 21:30:20]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-22 21:40:00]

rdevlin.cronin@chromium.org changed reviewers:
+ michaelpg@chromium.org

[Devlin, 2017-02-22 21:40:01]

Michael, mind taking a look?

Screenshots and demos available upon request.

[michaelpg, 2017-02-23 04:22:08]

https://codereview.chromium.org/2707083003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:4370: +          <ph
name="NUMBER_OF_VIEWS">$1<ex>2</ex></ph> more...
I think this should use number formatting, and probably account for plural
(probably matters in some languages)

see GetPluralStringF* in l10n_util

But it's a "developer" UI so... not hugely important.

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/item.html (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.html:37: display: flex;
Is there any convention you're using for when to use iron-flex attributes vs.
the raw CSS for layout?

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.html:106: padding: 8px 20px 8px 4px;
make RTL-friendly

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.html:130: transform:
translateX(-50%); /* Move back 50% of width so that the
nit: two spaces after ; (as above)

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.html:205:
if="[[computeInspectViewsHidden_(data.views)]]">
should this have a !, since it shows if it's not hidden?

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/item.js (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.js:239: return this.data.views &&
this.data.views.length > 0;
this seems like the inverse of what we want (unless I'm misunderstanding when
the "views" should appear)

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_item_test.js (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_item_test.js:30: text: '1 more…'},
why the fancy ellipsis?

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_item_test.js:139:
item.$$('#inspect-views paper-button:nth-of-type(0n + 2)'));
you don't need the 0n +
https://drafts.csswg.org/selectors-3/#nth-child-pseudo

[Devlin, 2017-02-23 19:09:21]

https://codereview.chromium.org/2707083003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:4370: +          <ph
name="NUMBER_OF_VIEWS">$1<ex>2</ex></ph> more...
On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> I think this should use number formatting, and probably account for plural
> (probably matters in some languages)
> 
> see GetPluralStringF* in l10n_util
> 
> But it's a "developer" UI so... not hugely important.

Hmm, interesting.  Unfortunately, we don't have a good way of getting a plural
string from JS (and instrumenting it doesn't seem trivial).  Given this is "not
hugely important", think it's okay to leave like this?

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/item.html (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.html:37: display: flex;
On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> Is there any convention you're using for when to use iron-flex attributes vs.
> the raw CSS for layout?

I always use the raw css unless someone expresses a preference for the iron-flex
version. :)

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.html:106: padding: 8px 20px 8px 4px;
On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> make RTL-friendly

Done.

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.html:130: transform:
translateX(-50%); /* Move back 50% of width so that the
On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> nit: two spaces after ; (as above)

Done.

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.html:205:
if="[[computeInspectViewsHidden_(data.views)]]">
On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> should this have a !, since it shows if it's not hidden?

Yes, done.  Funnily enough, my boolean dyslexia carried over into the
implementation, so it was actually doing the right thing.

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_extensions/item.js (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_extensions/item.js:239: return this.data.views &&
this.data.views.length > 0;
On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> this seems like the inverse of what we want (unless I'm misunderstanding when
> the "views" should appear)

Nope, you're right.  Fixed.

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_item_test.js (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_item_test.js:30: text: '1 more…'},
On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> why the fancy ellipsis?

That's apparently how we format the string in the localization process.  It
didn't seem worthwhile to instrument a textStartsWith: '1 more' to avoid it.

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_item_test.js:139:
item.$$('#inspect-views paper-button:nth-of-type(0n + 2)'));
On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> you don't need the 0n +
> https://drafts.csswg.org/selectors-3/#nth-child-pseudo

Done.

[michaelpg, 2017-02-27 01:29:50]

lgtm

https://codereview.chromium.org/2707083003/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:4370: +          <ph
name="NUMBER_OF_VIEWS">$1<ex>2</ex></ph> more...
On 2017/02/23 19:09:21, Devlin wrote:
> On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> > I think this should use number formatting, and probably account for plural
> > (probably matters in some languages)
> > 
> > see GetPluralStringF* in l10n_util
> > 
> > But it's a "developer" UI so... not hugely important.
> 
> Hmm, interesting.  Unfortunately, we don't have a good way of getting a plural
> string from JS (and instrumenting it doesn't seem trivial).  Given this is
"not
> hugely important", think it's okay to leave like this?

Yep, not worth the investigation here. It's an issue throughout our code base,
so I just like bring it up from time to time.

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_item_test.js (right):

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_item_test.js:30: text: '1 more…'},
On 2017/02/23 19:09:21, Devlin wrote:
> On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> > why the fancy ellipsis?
> 
> That's apparently how we format the string in the localization process.  It
> didn't seem worthwhile to instrument a textStartsWith: '1 more' to avoid it.

Cool, TIL.

https://codereview.chromium.org/2707083003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_item_test.js:139:
item.$$('#inspect-views paper-button:nth-of-type(0n + 2)'));
On 2017/02/23 19:09:21, Devlin wrote:
> On 2017/02/23 04:22:08, michaelpg incommunicado Th-F wrote:
> > you don't need the 0n +
> > https://drafts.csswg.org/selectors-3/#nth-child-pseudo
> 
> Done.

well, you did the one above instead ;-)

https://codereview.chromium.org/2707083003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_item_test.js (right):

https://codereview.chromium.org/2707083003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_item_test.js:139:
item.$$('#inspect-views paper-button:nth-of-type(0n + 2)'));
(2)

[Devlin, 2017-02-27 17:51:12]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 17:51:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 18:58:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 18:58:25]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-27 19:28:09]

https://codereview.chromium.org/2707083003/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/extensions/extension_item_test.js (right):

https://codereview.chromium.org/2707083003/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/extensions/extension_item_test.js:139:
item.$$('#inspect-views paper-button:nth-of-type(0n + 2)'));
On 2017/02/27 01:29:50, michaelpg wrote:
> (2)

D'oh!  Really done this time.

[Devlin, 2017-02-27 19:28:23]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-02-27 19:28:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2707083003/#ps80001
(title: "missed one")

[commit-bot: I haz the power, 2017-02-27 19:28:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 19:35:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488223703666150,
"parent_rev": "8d2150bcb3b87ee43b979e6ddda563e35b23077f", "commit_rev":
"4c12e06b9f81f8845fa504213ebc3077773ca444"}

[commit-bot: I haz the power, 2017-02-27 19:35:50]

Description was changed from

==========
[MD Extensions] Update Item Card UI

We have new mocks!  Update the item card UI to be more in-line with the
new specs.

BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Extensions] Update Item Card UI

We have new mocks!  Update the item card UI to be more in-line with the
new specs.

BUG=529395
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2707083003
Cr-Commit-Position: refs/heads/master@{#453297}
Committed:
https://chromium.googlesource.com/chromium/src/+/4c12e06b9f81f8845fa504213ebc...
==========

[commit-bot: I haz the power, 2017-02-27 19:35:51]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4c12e06b9f81f8845fa504213ebc...