[GlowUp] Round print_preview/ui/icons.html icons [chromium/src : main]
Lei Zhang (Gerrit)
Lei Zhang added 4 comments![]( "Open in Gerrit")
return loadTimeData.getBoolean('webuiRoundedIconsEnabled') ?Is a different CL going to set this boolean?
<g id="short-edge-custom" viewBox="0 0 26 20" fill-rule="evenodd">Is it necessary to rename these two?
These icons are copied from Polymer's iron-icons and kept in sorted order.Just to double check, does this comment still hold?
<g id="business-old"><path d="M12 7V3H2v18h20V7H12zM6 19H4v-2h2v2zm0-4H4v-2h2v2zm0-4H4V9h2v2zm0-4H4V5h2v2zm4 12H8v-2h2v2zm0-4H8v-2h2v2zm0-4H8V9h2v2zm0-4H8V5h2v2zm10 12h-8v-2h2v-2h-2v-2h2v-2h-2V9h8v10zm-2-8h-2v2h2v-2zm0 4h-2v2h2v-2z"></path></g>There's no naming conflict, so why rename the pre-existing icons? If the "-old" suffix is meant to be an indicator to mark the icons for future deletion, then how about adding a TODO instead?
Related details
- Emily Shack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emily Shack (Gerrit)
Emily Shack added 4 comments![]( "Open in Gerrit")
return loadTimeData.getBoolean('webuiRoundedIconsEnabled') ?Is a different CL going to set this boolean?
It's already set globally across WebUIs, see https://chromium-review.git.corp.google.com/c/chromium/src/+/8031801
<g id="short-edge-custom" viewBox="0 0 26 20" fill-rule="evenodd">Is it necessary to rename these two?
Yes. See https://docs.google.com/document/d/1QoI-9OzCHkxlKtDPHunUT66c1wKHwEsQqB5cx_kEpBQ/edit?usp=sharing, a big part of this effort is making it easy to bulk-update icons going forward, which means following a consistent naming schema. The -custom suffix indicates these icons don't map to anything on go/icons and therefore should not be subject to bulk updates. Without this suffix, it would be ambiguous to bulk update scripts whether this icon was meant to be custom or just hasn't had its naming standardized yet.
These icons are copied from Polymer's iron-icons and kept in sorted order.Just to double check, does this comment still hold?
My assumption is that this refers to icons in the cr:foo namespace, and those are slated for update as well though I don't have a CL out for that yet (I'm in the process of updating over 600 WebUI icons across likely ~40-ish CLs). These icons are also still in sorted order. So, I think this comment should hold.
<g id="business-old"><path d="M12 7V3H2v18h20V7H12zM6 19H4v-2h2v2zm0-4H4v-2h2v2zm0-4H4V9h2v2zm0-4H4V5h2v2zm4 12H8v-2h2v2zm0-4H8v-2h2v2zm0-4H8V9h2v2zm0-4H8V5h2v2zm10 12h-8v-2h2v-2h-2v-2h2v-2h-2V9h8v10zm-2-8h-2v2h2v-2zm0 4h-2v2h2v-2z"></path></g>There's no naming conflict, so why rename the pre-existing icons? If the "-old" suffix is meant to be an indicator to mark the icons for future deletion, then how about adding a TODO instead?
Because there are so many WebUI icons (over 600) most of this is being done via scripts vs. manually. It's much easier from a bulk update perspective to categorically add the -old suffix to everything and then add new icons with the correct naming, as well as later deleting everything with the -old suffix during cleanup, vs. keeping the original name of some icons alongside TODOs. Also, this matches how the corresponding Views icon migration (rolling out in M150) was done. Do you have concerns about renaming these?
Related details
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Lei Zhang
| Code-Review | +1 |
2 comments
<g id="short-edge-custom" viewBox="0 0 26 20" fill-rule="evenodd">Emily ShackIs it necessary to rename these two?
Yes. See https://docs.google.com/document/d/1QoI-9OzCHkxlKtDPHunUT66c1wKHwEsQqB5cx_kEpBQ/edit?usp=sharing, a big part of this effort is making it easy to bulk-update icons going forward, which means following a consistent naming schema. The -custom suffix indicates these icons don't map to anything on go/icons and therefore should not be subject to bulk updates. Without this suffix, it would be ambiguous to bulk update scripts whether this icon was meant to be custom or just hasn't had its naming standardized yet.
Thanks for the details to help me understand this is not just churn. This is a sign that the CL description is too vague. Some issues:
1. Not everyone has access to go/chrome-icon-modernization-dd, so how do future readers figure out what is going on and what this CL does?
2. Reading the TL;DR on go/chrome-icon-modernization-dd doesn't cover this.
Having a detailed CL description that explains the details of this CL would make this clearer.
<g id="business-old"><path d="M12 7V3H2v18h20V7H12zM6 19H4v-2h2v2zm0-4H4v-2h2v2zm0-4H4V9h2v2zm0-4H4V5h2v2zm4 12H8v-2h2v2zm0-4H8v-2h2v2zm0-4H8V9h2v2zm0-4H8V5h2v2zm10 12h-8v-2h2v-2h-2v-2h2v-2h-2V9h8v10zm-2-8h-2v2h2v-2zm0 4h-2v2h2v-2z"></path></g>Emily ShackThere's no naming conflict, so why rename the pre-existing icons? If the "-old" suffix is meant to be an indicator to mark the icons for future deletion, then how about adding a TODO instead?
Because there are so many WebUI icons (over 600) most of this is being done via scripts vs. manually. It's much easier from a bulk update perspective to categorically add the -old suffix to everything and then add new icons with the correct naming, as well as later deleting everything with the -old suffix during cleanup, vs. keeping the original name of some icons alongside TODOs. Also, this matches how the corresponding Views icon migration (rolling out in M150) was done. Do you have concerns about renaming these?
When I wrote the comment initially, I had no idea what the intent was. (See other comment about CL description.) I was also not involved in the Views icon migration, so this is unfamiliar to me. If this is the norm, then ok.
Out of curiosity, is the use of the "-old" suffix documented anywhere? Is there a task filed for their deletion?
Related details
- Emily Shack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emily Shack (Gerrit)
Emily Shack added 2 comments![]( "Open in Gerrit")
<g id="short-edge-custom" viewBox="0 0 26 20" fill-rule="evenodd">Emily ShackIs it necessary to rename these two?
Lei ZhangYes. See https://docs.google.com/document/d/1QoI-9OzCHkxlKtDPHunUT66c1wKHwEsQqB5cx_kEpBQ/edit?usp=sharing, a big part of this effort is making it easy to bulk-update icons going forward, which means following a consistent naming schema. The -custom suffix indicates these icons don't map to anything on go/icons and therefore should not be subject to bulk updates. Without this suffix, it would be ambiguous to bulk update scripts whether this icon was meant to be custom or just hasn't had its naming standardized yet.
Thanks for the details to help me understand this is not just churn. This is a sign that the CL description is too vague. Some issues:
1. Not everyone has access to go/chrome-icon-modernization-dd, so how do future readers figure out what is going on and what this CL does?
2. Reading the TL;DR on go/chrome-icon-modernization-dd doesn't cover this.Having a detailed CL description that explains the details of this CL would make this clearer.
Opened up go/chrome-icon-modernization-dd to all of Alphabet and added to the TL;DR to clarify both the suffixes and the intent to enable future bulk updates. Also expanded on this in the CL description.
<g id="business-old"><path d="M12 7V3H2v18h20V7H12zM6 19H4v-2h2v2zm0-4H4v-2h2v2zm0-4H4V9h2v2zm0-4H4V5h2v2zm4 12H8v-2h2v2zm0-4H8v-2h2v2zm0-4H8V9h2v2zm0-4H8V5h2v2zm10 12h-8v-2h2v-2h-2v-2h2v-2h-2V9h8v10zm-2-8h-2v2h2v-2zm0 4h-2v2h2v-2z"></path></g>Emily ShackThere's no naming conflict, so why rename the pre-existing icons? If the "-old" suffix is meant to be an indicator to mark the icons for future deletion, then how about adding a TODO instead?
Lei ZhangBecause there are so many WebUI icons (over 600) most of this is being done via scripts vs. manually. It's much easier from a bulk update perspective to categorically add the -old suffix to everything and then add new icons with the correct naming, as well as later deleting everything with the -old suffix during cleanup, vs. keeping the original name of some icons alongside TODOs. Also, this matches how the corresponding Views icon migration (rolling out in M150) was done. Do you have concerns about renaming these?
When I wrote the comment initially, I had no idea what the intent was. (See other comment about CL description.) I was also not involved in the Views icon migration, so this is unfamiliar to me. If this is the norm, then ok.
Out of curiosity, is the use of the "-old" suffix documented anywhere? Is there a task filed for their deletion?
Updated the design doc to make this more explicit. Removal is tracked here: crbug.com/532681362
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emily Shack (Gerrit)
Emily Shack voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[GlowUp] Round print_preview/ui/icons.html icons
See go/chrome-icon-modernization-dd. This is part of a larger effort to
standardize icon naming, enabling easier bulk updates in the future, as
well as applying 100% roundedness to all existing icons.
- M chrome/browser/resources/print_preview/data/destination.ts
- M chrome/browser/resources/print_preview/ui/destination_select.ts
- M chrome/browser/resources/print_preview/ui/duplex_settings.ts
- M chrome/browser/resources/print_preview/ui/header.html.ts
- M chrome/browser/resources/print_preview/ui/header.ts
- M chrome/browser/resources/print_preview/ui/icons.html
- M chrome/test/data/webui/print_preview/destination_select_test.ts
Code-Review: +1 by Lei Zhang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |