New issue
Advanced search Search tips

Issue 736891 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Replace remaining usages of paper-icon-button with paper-icon-button-light.

Project Member Reported by dpa...@chromium.org, Jun 26 2017

Issue description

This was already done for MD Settings, but there are more usages of paper-icon-button outside of Settings, see [1]. Having two ways of achieving the same thing (paper-icon-button VS paper-icon-button-light) is superfluous and migrating everything to the "light" version will allow us to stop including paper-icon-button-light in Chrome's binary.

https://cs.chromium.org/search/?q=paper-icon-button.html+-third_party+-infra&type=cs
 

Comment 1 by dpa...@chromium.org, Jun 26 2017

Status: Assigned (was: Untriaged)
Owner: dschuyler@chromium.org
Scott is ok with me owning this bug.
Cc: scottchen@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ede4e786c32ee7cb5217520619340a8dd2e8f267

commit ede4e786c32ee7cb5217520619340a8dd2e8f267
Author: Dave Schuyler <dschuyler@chromium.org>
Date: Tue Jul 11 01:04:03 2017

[MD settings] removed old references to paper-icon-button

This CL removes some old references to paper-icon-button within the
Md settings code. Removing these references doesn't appear to affect the
UI.

Bug: 736891
Change-Id: I09389ca5ffbe33e7b2057dd3fbc711abe3774715
Reviewed-on: https://chromium-review.googlesource.com/564204
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485473}
[modify] https://crrev.com/ede4e786c32ee7cb5217520619340a8dd2e8f267/chrome/browser/resources/settings/internet_page/internet_known_networks_page.html
[modify] https://crrev.com/ede4e786c32ee7cb5217520619340a8dd2e8f267/chrome/browser/resources/settings/languages_page/languages_page.js
[modify] https://crrev.com/ede4e786c32ee7cb5217520619340a8dd2e8f267/chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
[modify] https://crrev.com/ede4e786c32ee7cb5217520619340a8dd2e8f267/chrome/browser/resources/settings/settings_main/settings_main.html
[modify] https://crrev.com/ede4e786c32ee7cb5217520619340a8dd2e8f267/chrome/browser/resources/settings/settings_page/settings_subpage.html
[modify] https://crrev.com/ede4e786c32ee7cb5217520619340a8dd2e8f267/chrome/browser/resources/settings/settings_vars_css.html

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47231d081a55b17cb12cf4c633202873bbcc2234

commit 47231d081a55b17cb12cf4c633202873bbcc2234
Author: Dave Schuyler <dschuyler@chromium.org>
Date: Mon Jul 17 20:15:45 2017

[MD settings] change paper-icon-button to *-light in cr_dialog

This CL changes the clear (X button) in the upper right of a cr_dialog
from a paper-icon-button to a paper-icon-button-light. It also moves
some of the paper-icon-button-light styling from settings to cr CSS so
that it can apply in cr_*.

Bug: 736891
Change-Id: Idc211b5dc42d18261d5e0906fea6f5d385421f0e
Reviewed-on: https://chromium-review.googlesource.com/567728
Reviewed-by: Scott Chen <scottchen@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Dave Schuyler <dschuyler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487217}
[modify] https://crrev.com/47231d081a55b17cb12cf4c633202873bbcc2234/ui/webui/resources/cr_elements/cr_dialog/cr_dialog.html
[modify] https://crrev.com/47231d081a55b17cb12cf4c633202873bbcc2234/ui/webui/resources/cr_elements/cr_icons_css.html
[modify] https://crrev.com/47231d081a55b17cb12cf4c633202873bbcc2234/ui/webui/resources/cr_elements/shared_style_css.html

Cc: -scottchen@chromium.org
Owner: scottchen@chromium.org
Components: UI>Browser>WebUI
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: aee@chromium.org
aee@ I think this would be part of the 'iron-icon or background svg' investigation.

Comment 10 by aee@chromium.org, Today (2 hours ago)

Status: Started (was: Assigned)

Comment 11 by dpapad@google.com, Today (2 hours ago)

FWIW, the distinction between paper-icon-button and paper-icon-button-light is not as clear as it was when I filed this bug originally. Explaining below:

At the time
 - paper-icon-button was a wrapper around <button>. 
 - paper-icon-button-light was extending the native <button>

This is what made the latter be a "lighter" element in terms of performance. After the migration to paper-icon-button-light 2.x that is no longer the case. paper-icon-button-light is now also a wrapper, since Polymer2 does not support extending native HTML elements anymore.

FWIW, I think the ideal end goal would be to replace both paper-icon-button and paper-icon-button-light with a cr-icon-button element. If you think removing paper-icon-button is a step in that direction go ahead, otherwise let's investigate how a cr-icon-button element would look first, before doing any migrations.

Comment 12 by dpa...@chromium.org, Today (2 hours ago)

Cc: dbeam@chromium.org

Sign in to add a comment