Add material design toggle ripple element.

Add material design toggle ripple element.

Add material design toggle ripple element and use it in the gear menu button.

BUG=499160
TEST=none

Committed: https://crrev.com/e3a3f806542a38b0219745967a6a4bb4f9f364a7
Cr-Commit-Position: refs/heads/master@{#334565}

[yawano, 2015-06-10 07:26:22]

yawano@chromium.org changed reviewers:
+ fukino@chromium.org

[yawano, 2015-06-10 07:26:22]

PTAL. Thank you!

[fukino, 2015-06-10 08:28:53]

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager_resources.grd (right):

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager_resources.grd:182: <include
name="IDR_GALLERY_ELEMENTS_GALLERY_TOGGLE_BUTTON_HTML"
file="gallery/elements/gallery_toggle_button.html" type="BINDATA" />
It'd be better if file_manager and gallery can share this element.

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
File ui/file_manager/gallery/elements/gallery_toggle_button.js (right):

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
ui/file_manager/gallery/elements/gallery_toggle_button.js:24: observer:
'onActivatedChanged_'
nit: 'activatedChanged' or 'activatedChanged_' looks more consistent with
existing code.

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
ui/file_manager/gallery/elements/gallery_toggle_button.js:32: onTapped_:
function() {
I'm not sure if TAP is the trigger for this animation.
I thought the effect was for FOCUSED state. If so, tab key etc. can trigger the
effect.
Maybe we should clarify the spec.

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
ui/file_manager/gallery/elements/gallery_toggle_button.js:127: 'border-radius':
'2.56%',
nit: These single quotation marks looks unnecessary.

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
ui/file_manager/gallery/elements/gallery_toggle_button.js:132: 'border-radius':
'50%',
ditto

[yawano, 2015-06-10 08:55:19]

Thank you for the review!

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager_resources.grd (right):

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager_resources.grd:182: <include
name="IDR_GALLERY_ELEMENTS_GALLERY_TOGGLE_BUTTON_HTML"
file="gallery/elements/gallery_toggle_button.html" type="BINDATA" />
Yes, I agree with it. We are going to extract this to a shared module. Is it
okay to do it in a separate CL? i.e. lands as gallery_toggle_button first and
move it to a shared module with renaming it.

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
File ui/file_manager/gallery/elements/gallery_toggle_button.js (right):

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
ui/file_manager/gallery/elements/gallery_toggle_button.js:24: observer:
'onActivatedChanged_'
On 2015/06/10 08:28:52, fukino wrote:
> nit: 'activatedChanged' or 'activatedChanged_' looks more consistent with
> existing code.

Done.

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
ui/file_manager/gallery/elements/gallery_toggle_button.js:127: 'border-radius':
'2.56%',
If we remove single quotes, it becomes syntax error.
Key: symbol cannot contain '-'.
Value: % is parsed as operator.

[yawano, 2015-06-11 03:21:31]

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
File ui/file_manager/gallery/elements/gallery_toggle_button.js (right):

https://codereview.chromium.org/1170913006/diff/1/ui/file_manager/gallery/ele...
ui/file_manager/gallery/elements/gallery_toggle_button.js:32: onTapped_:
function() {
The active state here means "pressed", e.g. showing context menu. It does not
include "focus".

[yawano, 2015-06-11 05:53:12]

As we discussed in other place, extract as the ripple, move it to file_manager,
and use it for gear menu button.

PTAL. Thank you!

[yawano, 2015-06-11 05:55:30]

yawano@chromium.org changed reviewers:
+ dbeam@chromium.org

[yawano, 2015-06-11 05:55:31]

@dbeam: PTAL ui/webui/resources/js/cr/ui/menu_button.js. Thank you!

[fukino, 2015-06-11 06:23:31]

https://codereview.chromium.org/1170913006/diff/40001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/1170913006/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:231: this.dispatchEvent(new
Event('menuhide'));

I think adding public event to cr.ui.Menu should be a separate issue and a
separate CL.
(For example, an issue "Add an event to cr.ui.Menu to notify that the menu is
hidden.", which we want to implement our new UI spec.)

'menuhide' should be more symmetric to 'menushow'. That is, it should be an
instance of UIEvent and have the same property with 'menushow'.

[yawano, 2015-06-11 07:07:21]

Thank you for the review!

https://codereview.chromium.org/1170913006/diff/40001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/menu_button.js (right):

https://codereview.chromium.org/1170913006/diff/40001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/menu_button.js:231: this.dispatchEvent(new
Event('menuhide'));
I tried to get hide event in files.app by using Object.observe, but it hasn't
worked since Object.observe does not notify for attribute changes. We need to
dispatch hide event here to implement this.

I'll create another CL to dispatch menuhide UIEvent.

[yawano, 2015-06-11 07:30:57]

Extracted changes to menu_button.js as http://crrev.com/1178783003. We are going
to rebase this CL on top of the CL after it has landed.

[yawano, 2015-06-11 07:32:25]

yawano@chromium.org changed reviewers:
- dbeam@chromium.org

[yawano, 2015-06-11 07:32:25]

Remove @dbeam from reviewers in this CL since we extracted changes to
menu_button.js to another CL.

[fukino, 2015-06-12 06:28:48]

Thanks!

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/externs...
File ui/file_manager/externs/files_elements.js (right):

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/externs...
ui/file_manager/externs/files_elements.js:14: this.activated;
Could you use the same format as other externs file?
i.e.

/** */
function FilesToggleRipple() {}

/** */
FilesToggleRipple.prototype.activated;

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/main.html (right):

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/file_ma...
ui/file_manager/file_manager/main.html:279:
<files-toggle-ripple></files-toggle-ripple>
Let's wait for the spec clarification, to avoid showing user intermediate state
(only Gear button has the effect).

[yawano, 2015-06-16 06:36:40]

Thank you for the review! PTAL.

Note: With a new patch set, I changed CSS to make the ripple size same with the
element size. I believe it should be easier to use.

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/externs...
File ui/file_manager/externs/files_elements.js (right):

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/externs...
ui/file_manager/externs/files_elements.js:14: this.activated;
On 2015/06/12 06:28:48, fukino wrote:
> Could you use the same format as other externs file?
> i.e.
> 
> /** */
> function FilesToggleRipple() {}
> 
> /** */
> FilesToggleRipple.prototype.activated;

Done.

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/main.html (right):

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/file_ma...
ui/file_manager/file_manager/main.html:279:
<files-toggle-ripple></files-toggle-ripple>
Fixed sort and search button in this CL.

For open and cloud import button, I'll do them with another CL since they
require some additional work.

[fukino, 2015-06-16 07:34:16]

Thanks! lgtm with a nit.

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/main.html (right):

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/file_ma...
ui/file_manager/file_manager/main.html:279:
<files-toggle-ripple></files-toggle-ripple>
On 2015/06/16 06:36:40, yawano wrote:
> Fixed sort and search button in this CL.
> 
> For open and cloud import button, I'll do them with another CL since they
> require some additional work.

OK.
But please make sure the toolbar buttons with sticking state have consistent
style in M45.

https://codereview.chromium.org/1170913006/diff/110001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_toggle_ripple.js
(right):

https://codereview.chromium.org/1170913006/diff/110001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_toggle_ripple.js:53: {
nit: Indent level should be 2 after [.

[yawano, 2015-06-16 07:38:57]

Thank you!

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/main.html (right):

https://codereview.chromium.org/1170913006/diff/60001/ui/file_manager/file_ma...
ui/file_manager/file_manager/main.html:279:
<files-toggle-ripple></files-toggle-ripple>
On 2015/06/16 07:34:16, fukino wrote:
> On 2015/06/16 06:36:40, yawano wrote:
> > Fixed sort and search button in this CL.
> > 
> > For open and cloud import button, I'll do them with another CL since they
> > require some additional work.
> 
> OK.
> But please make sure the toolbar buttons with sticking state have consistent
> style in M45.

Acknowledged.

https://codereview.chromium.org/1170913006/diff/110001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_toggle_ripple.js
(right):

https://codereview.chromium.org/1170913006/diff/110001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_toggle_ripple.js:53: {
On 2015/06/16 07:34:16, fukino wrote:
> nit: Indent level should be 2 after [.

Done.

[yawano, 2015-06-16 07:39:01]

The CQ bit was checked by yawano@chromium.org

[yawano, 2015-06-16 07:39:02]

The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/1170913006/#ps130001
(title: "Fix format.")

[commit-bot: I haz the power, 2015-06-16 07:39:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1170913006/130001

[commit-bot: I haz the power, 2015-06-16 08:15:08]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-06-16 08:16:06]

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