Settings People Revamp: Implement Change People preview pane

Settings People Revamp: Implement Change People preview pane

This patch:
 1. Adds a camera image icon. Currently just sets a cameraActive
    property and hides the static image preview pane, but functionality
    will expand in subsquent patches.
 2. Adds a preview pane as described in the mocks for the selected
    image.

BUG=563721
Committed: https://crrev.com/4e96e1117d92789cd1012812d9be9b2ddee626aa
Cr-Commit-Position: refs/heads/master@{#370796}

[tommycli, 2016-01-21 01:35:10]

tommycli@chromium.org changed reviewers:
+ dpapad@chromium.org

[tommycli, 2016-01-21 01:35:10]

dpapad: PTAL, thanks!

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.css (right):

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.css:20:
#preview-pane img {
These CSS Changes are inspired by the existing CSS for old Options. The mocks
haven't changed the general layout much.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

[dpapad, 2016-01-21 18:11:40]

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.css (right):

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.css:10:
#availableIcons paper-button {
All other CSS classes and IDs in this file are using this-naming-style instead
of thisNamingStyle.

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:56:
cameraImageUrl: {
Does this URL need to be public (all other URLs in this file are private)?

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:64: * @type
{string}
@private or remove the underscore from the name.

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:114:
this.cameraActive = false;
Would it make sense to set the value of this boolean in an observer of the
selectedImageUrl_ property instead of repeating it three times?

https://codereview.chromium.org/1610973002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js
(right):

https://codereview.chromium.org/1610973002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:82:
expectFalse(changePicture.cameraActive);
Why are we using expect() here instead of assert()? It seems that if
changePicture.cameraActive is true here, then the rest of this test should not
execute, which is achieved with assert().

[tommycli, 2016-01-21 18:47:05]

dpapad: thank you!

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.css (right):

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.css:10:
#availableIcons paper-button {
On 2016/01/21 18:11:40, dpapad wrote:
> All other CSS classes and IDs in this file are using this-naming-style instead
> of thisNamingStyle.

Done.

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:56:
cameraImageUrl: {
On 2016/01/21 18:11:40, dpapad wrote:
> Does this URL need to be public (all other URLs in this file are private)?

Done. Actually I think not.

The eventual goal is to have an embedded element. I think this can still be
private.

<camera-image camera-image-url="{{cameraImageUrl_}}">

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:64: * @type
{string}
On 2016/01/21 18:11:40, dpapad wrote:
> @private or remove the underscore from the name.

Done.

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:114:
this.cameraActive = false;
On 2016/01/21 18:11:40, dpapad wrote:
> Would it make sense to set the value of this boolean in an observer of the
> selectedImageUrl_ property instead of repeating it three times?

Hmm.. I'm not sure yet.

Basically I'm still not sure how to reconcile selectedImageUrl_ with
cameraActive_ yet. The two seem a little redundant. I think this will become
clear in the next patchset, so would it be alright if I left it as-is for now,
but came to a decision next patch?

https://codereview.chromium.org/1610973002/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js
(right):

https://codereview.chromium.org/1610973002/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:82:
expectFalse(changePicture.cameraActive);
On 2016/01/21 18:11:40, dpapad wrote:
> Why are we using expect() here instead of assert()? It seems that if
> changePicture.cameraActive is true here, then the rest of this test should not
> execute, which is achieved with assert().

Done.

[dpapad, 2016-01-21 18:55:47]

lgtm

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:114:
this.cameraActive = false;
On 2016/01/21 at 18:47:05, tommycli wrote:
> On 2016/01/21 18:11:40, dpapad wrote:
> > Would it make sense to set the value of this boolean in an observer of the
> > selectedImageUrl_ property instead of repeating it three times?
> 
> Hmm.. I'm not sure yet.
> 
> Basically I'm still not sure how to reconcile selectedImageUrl_ with
cameraActive_ yet. The two seem a little redundant. I think this will become
clear in the next patchset, so would it be alright if I left it as-is for now,
but came to a decision next patch?

SG.

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:166: * @param
{!Event} event
We are not using the event parameter. Can it be removed?

https://codereview.chromium.org/1610973002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.html (right):

https://codereview.chromium.org/1610973002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.html:20:
active="{{isActiveImage_(cameraActive_, profileImageUrl_, selectedImageUrl_)}}"
Nit: cameraActive_ is passed in all calls to isActiveImage_. How about not
passing it as a param, instead just referring to it from within the function
directly?

https://codereview.chromium.org/1610973002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.html:41: <img
id="previewImage" src="[[selectedImageUrl_]]"
Nit: s/previewImage/preview-image

[tommycli, 2016-01-21 19:08:03]

dpapad: thanks!

sorry to reject some nits below. trying to follow style guide.

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.js:166: * @param
{!Event} event
On 2016/01/21 18:55:47, dpapad wrote:
> We are not using the event parameter. Can it be removed?

Done.

https://codereview.chromium.org/1610973002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.html (right):

https://codereview.chromium.org/1610973002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.html:20:
active="{{isActiveImage_(cameraActive_, profileImageUrl_, selectedImageUrl_)}}"
On 2016/01/21 18:55:47, dpapad wrote:
> Nit: cameraActive_ is passed in all calls to isActiveImage_. How about not
> passing it as a param, instead just referring to it from within the function
> directly?

Following Polymer #3 on style guide here:
https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...

https://codereview.chromium.org/1610973002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.html:41: <img
id="previewImage" src="[[selectedImageUrl_]]"
On 2016/01/21 18:55:47, dpapad wrote:
> Nit: s/previewImage/preview-image

Since we use foo.$.previewImage in the test, we make that ID camel case per HTML
#3 in style guide above.

[Dan Beam, 2016-01-21 19:33:28]

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/change_picture.css (right):

https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/change_picture.css:10:
#availableIcons paper-button {
On 2016/01/21 18:47:05, tommycli wrote:
> On 2016/01/21 18:11:40, dpapad wrote:
> > All other CSS classes and IDs in this file are using this-naming-style
instead
> > of thisNamingStyle.
> 
> Done.

fwiw: I dropped the presubmit check for ids-like-this a while ago so
this.$.idsLikeThis would be easier to do, and just updated the style guide[1].

[1]
https://sites.google.com/a/chromium.org/dev/developers/web-development-style-...

[tommycli, 2016-01-21 20:24:29]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-01-21 20:24:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/1610973002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-01-21 20:25:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1610973002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1610973002/60001

[tommycli, 2016-01-21 20:29:05]

On 2016/01/21 19:33:28, Dan Beam wrote:
>
https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/people_page/change_picture.css (right):
> 
>
https://codereview.chromium.org/1610973002/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/people_page/change_picture.css:10:
> #availableIcons paper-button {
> On 2016/01/21 18:47:05, tommycli wrote:
> > On 2016/01/21 18:11:40, dpapad wrote:
> > > All other CSS classes and IDs in this file are using this-naming-style
> instead
> > > of thisNamingStyle.
> > 
> > Done.
> 
> fwiw: I dropped the presubmit check for ids-like-this a while ago so
> this.$.idsLikeThis would be easier to do, and just updated the style guide[1].
> 
> [1]
>
https://sites.google.com/a/chromium.org/dev/developers/web-development-style-...

Ah. Excellent. I didn't know about that style guide. THx.

[commit-bot: I haz the power, 2016-01-21 21:45:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-21 21:47:10]

Description was changed from

==========
Settings People Revamp: Implement Change People preview pane

This patch:
 1. Adds a camera image icon. Currently just sets a cameraActive
    property and hides the static image preview pane, but functionality
    will expand in subsquent patches.
 2. Adds a preview pane as described in the mocks for the selected
    image.

BUG=563721
==========

to

==========
Settings People Revamp: Implement Change People preview pane

This patch:
 1. Adds a camera image icon. Currently just sets a cameraActive
    property and hides the static image preview pane, but functionality
    will expand in subsquent patches.
 2. Adds a preview pane as described in the mocks for the selected
    image.

BUG=563721

Committed: https://crrev.com/4e96e1117d92789cd1012812d9be9b2ddee626aa
Cr-Commit-Position: refs/heads/master@{#370796}
==========

[commit-bot: I haz the power, 2016-01-21 21:47:11]

Patchset 4 (id:??) landed as
https://crrev.com/4e96e1117d92789cd1012812d9be9b2ddee626aa
Cr-Commit-Position: refs/heads/master@{#370796}