Gallery.app: make edit button toggleable.

Gallery.app: make edit button toggleable.

BUG=512760
TEST=none

Committed: https://crrev.com/03929ad61c27d01af9e3a655b6d81af3ea107c73
Cr-Commit-Position: refs/heads/master@{#340842}

[yawano, 2015-07-27 12:03:42]

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

[yawano, 2015-07-27 12:03:43]

PTAL. Thank you!

[hirono, 2015-07-27 12:28:00]

https://codereview.chromium.org/1255143003/diff/1/ui/file_manager/gallery/css...
File ui/file_manager/gallery/css/gallery.css (right):

https://codereview.chromium.org/1255143003/diff/1/ui/file_manager/gallery/css...
ui/file_manager/gallery/css/gallery.css:613: .gallery > .toolbar .button.edit >
.icon {
Now we have three selectors for buttons here (paper-button, .button, button) and
it is a bit confusing. Can we remove .button class and introduce .toggle for
buttons that can be toggled? After that we can just use paper-button and
button.toggle. WDYT?

[yawano, 2015-07-28 01:58:01]

https://codereview.chromium.org/1255143003/diff/1/ui/file_manager/gallery/css...
File ui/file_manager/gallery/css/gallery.css (right):

https://codereview.chromium.org/1255143003/diff/1/ui/file_manager/gallery/css...
ui/file_manager/gallery/css/gallery.css:613: .gallery > .toolbar .button.edit >
.icon {
SGTM. I agree with that current situation is confusing.

The reason that I added .button class is for simplifying selectors in JS side.
If we query element by class name, we don't need to change JS side code when we
change the elements.

But even if we use class name for a selector, we need to know which element it
is in JS side (actually, we change JS side code in this CL).

I'll remove .button class and unify to use element names.

[yawano, 2015-07-28 02:20:28]

PTAL. Thank you!

https://codereview.chromium.org/1255143003/diff/20001/ui/file_manager/gallery...
File ui/file_manager/gallery/gallery.html (right):

https://codereview.chromium.org/1255143003/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/gallery.html:40: <button class="toggle edit"
i18n-values="title:GALLERY_EDIT">
While this will be preference, is it recommended to add a class for annotation?
(i.e. add a class which is not used by any selector just for comment). WDYT?

[hirono, 2015-07-28 04:53:36]

https://codereview.chromium.org/1255143003/diff/20001/ui/file_manager/gallery...
File ui/file_manager/gallery/gallery.html (right):

https://codereview.chromium.org/1255143003/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/gallery.html:40: <button class="toggle edit"
i18n-values="title:GALLERY_EDIT">
On 2015/07/28 02:20:28, yawano wrote:
> While this will be preference, is it recommended to add a class for
annotation?
> (i.e. add a class which is not used by any selector just for comment). WDYT?

It's fine to remove class. Sorry for confusion. My original idea was to make the
difference between paper-button and button clear.
How about adding explicit HTML comment that explains why we can not use paper
button here?

[yawano, 2015-07-28 06:41:47]

Thank you for the review! PTAL.

https://codereview.chromium.org/1255143003/diff/20001/ui/file_manager/gallery...
File ui/file_manager/gallery/gallery.html (right):

https://codereview.chromium.org/1255143003/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/gallery.html:40: <button class="toggle edit"
i18n-values="title:GALLERY_EDIT">
On 2015/07/28 04:53:36, hirono wrote:
> On 2015/07/28 02:20:28, yawano wrote:
> > While this will be preference, is it recommended to add a class for
> annotation?
> > (i.e. add a class which is not used by any selector just for comment). WDYT?
> 
> It's fine to remove class. Sorry for confusion. My original idea was to make
the
> difference between paper-button and button clear.
> How about adding explicit HTML comment that explains why we can not use paper
> button here?

Done.

[hirono, 2015-07-28 07:20:11]

lgtm!

[yawano, 2015-07-28 07:20:46]

On 2015/07/28 07:20:11, hirono wrote:
> lgtm!

Thank you!

[yawano, 2015-07-28 07:20:49]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-07-28 07:21:04]

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

[commit-bot: I haz the power, 2015-07-28 08:29:38]

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

[commit-bot: I haz the power, 2015-07-28 08:29:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yawano, 2015-07-29 02:02:23]

@hirono: Fixed failed test cases. PTAL again. Thank you!

[hirono, 2015-07-29 02:17:39]

lgtm!

[yawano, 2015-07-29 02:24:26]

On 2015/07/29 02:17:39, hirono wrote:
> lgtm!

Thank you!

[yawano, 2015-07-29 02:24:30]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2015-07-29 02:24:37]

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

[commit-bot: I haz the power, 2015-07-29 02:29:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-29 02:30:13]

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