Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2019053002:
Changed the icons for "Manage people" and sync errors (Closed)
DescriptionChanged two icons for desktop user menu on Linux/Windows:
1. Changed the icon for the "Manage people" button;
2. Changed the icon for sync error on the titlebar.
See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnqik/edit?ts=57445a70#heading=h.6xesoh23gozz
This CL is dependent on https://codereview.chromium.org/2028473002/
BUG=615893
Committed: https://crrev.com/b877548e091c72b836324d65c4714e7461b53a45
Cr-Commit-Position: refs/heads/master@{#398141}
Patch Set 1 : Manage people icon update #Patch Set 2 : Sync error icon update #Patch Set 3 : Sync error icon color update #Patch Set 4 : Split to be icon usage only #Patch Set 5 : Removed unnecessary changes #Patch Set 6 : Put changes behind flag #
Total comments: 1
Patch Set 7 : Fixed nit #
Messages
Total messages: 36 (16 generated)
The CQ bit was checked by janeliulwq@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2019053002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2019053002/1
The CQ bit was unchecked by janeliulwq@google.com
Description was changed from ========== Changed the icon of "Manage people" on desktop user menu. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG= ========== to ========== Added / changed two icons for desktop user menu on both Linux and Mac: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG= ==========
janeliulwq@google.com changed reviewers: + rogerta@chromium.org
janeliulwq@google.com changed required reviewers: + rogerta@chromium.org
Hi Jane, Let's separate this CL into smaller, more self contained parts. Make the the three ui/gfx files their own CL. This simply adds new svg icons for use later. In the CL comment, refer to the design doc and even a link to next CL so that the reviewers are aware of how these icons will be used. I can show you how to use git to make one CL depend on another even when it is not committed yet, this is a very useful technique. Next make a CL with the two chrome/browser/ui/views files that use the new svg files. Please also attached screenshots of what they look like to the tracking bug (see below). The rest of the filed are for mac. I would suggest not committing these file for now. A recent goal in chrome is to replace all the png images (or as many as possible anyway) with svg files. Let's not add these four png files for now just to support the mac, which will eventually be replaced with the new svg icons anyway. Also, let's track all these changes with a bug in crbug (Anthony should tell us what it is). In each CL you can add the number after the BUG= line. Does that make sense?
Description was changed from ========== Added / changed two icons for desktop user menu on both Linux and Mac: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... BUG= ========== to ========== Changed two icons for desktop user menu on Linux/Windows: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2028473002/ BUG= ==========
On 2016/05/30 19:37:38, Roger Tawa wrote: > Hi Jane, > > Let's separate this CL into smaller, more self contained parts. > > Make the the three ui/gfx files their own CL. This simply adds new svg icons > for use later. In the CL comment, refer to the design doc and even a link to > next CL so that the reviewers are aware of how these icons will be used. I can > show you how to use git to make one CL depend on another even when it is not > committed yet, this is a very useful technique. > > Next make a CL with the two chrome/browser/ui/views files that use the new svg > files. Please also attached screenshots of what they look like to the tracking > bug (see below). > > The rest of the filed are for mac. I would suggest not committing these file > for now. A recent goal in chrome is to replace all the png images (or as many > as possible anyway) with svg files. Let's not add these four png files for now > just to support the mac, which will eventually be replaced with the new svg > icons anyway. > > Also, let's track all these changes with a bug in crbug (Anthony should tell us > what it is). In each CL you can add the number after the BUG= line. > > Does that make sense? Makes sense! I have split them into two CLs. Let me know if anything else needs to be updated/corrected (other than the bug part since I don't have it yet).
lgtm Thanks Jane. Please add bug number to CL description and upload screenshots of the new buttons to the bug before sending for owner review.
Description was changed from ========== Changed two icons for desktop user menu on Linux/Windows: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2028473002/ BUG= ========== to ========== Changed two icons for desktop user menu on Linux/Windows: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2028473002/ BUG=615893 ==========
janeliulwq@google.com changed reviewers: + sky@chromium.org
janeliulwq@google.com changed required reviewers: + sky@chromium.org
On 2016/05/30 21:52:53, Jane wrote: I just thought of some thing. Is there any code still using the account-box icon, if not maybe it should be removed from the icon list .
On 2016/05/30 22:06:47, (NOT FOR CODE REVIEWS) wrote: > On 2016/05/30 21:52:53, Jane wrote: > > I just thought of some thing. Is there any code still using the account-box > icon, if not maybe it should be removed from the icon list . Yes there is, I remembered to check this time. The switch user dialog has it on the bottom.
LGTM
estade@chromium.org changed reviewers: + estade@chromium.org
lgtm but I'd prefer if you added resources in the same CL that used the resources. Easier to see how they're used and the two CLs don't make sense without the other (they are one atomic unit).
On 2016/06/02 00:12:05, Evan Stade wrote: > lgtm but I'd prefer if you added resources in the same CL that used the > resources. Easier to see how they're used and the two CLs don't make sense > without the other (they are one atomic unit). Thanks for the input Evan, however could you LGTM the other CL too please (that you are an owner of)? Thanks!
On 2016/06/02 00:15:09, Jane wrote: > On 2016/06/02 00:12:05, Evan Stade wrote: > > lgtm but I'd prefer if you added resources in the same CL that used the > > resources. Easier to see how they're used and the two CLs don't make sense > > without the other (they are one atomic unit). > > Thanks for the input Evan, however could you LGTM the other CL too please (that > you are an owner of)? Thanks! oops, I meant to put that comment on the other CL.
janeliulwq@google.com changed reviewers: + anthonyvd@chromium.org - estade@chromium.org
This CL was LGTM'ed before we established that all the changes related to bug 615893 need to be put behind a flag. The latest patch now includes the "if" code related to the flag, so please take a look again, thanks!
Hi Roger and sky, a gentle reminder to LGTM my CL again since I added the change behind a flag. Thanks!
SLGTM
rogerta@chromium.org changed reviewers: + estade@chromium.org
lgtm https://codereview.chromium.org/2019053002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/2019053002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/new_avatar_button.cc:224: gfx::kGoogleYellow700)); Nit: 223 and 224 may need to be indented a bit more, like the old code.
The CQ bit was checked by janeliulwq@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, estade@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2019053002/#ps120001 (title: "Fixed nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2019053002/120001
Message was sent while issue was closed.
Description was changed from ========== Changed two icons for desktop user menu on Linux/Windows: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2028473002/ BUG=615893 ========== to ========== Changed two icons for desktop user menu on Linux/Windows: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2028473002/ BUG=615893 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Changed two icons for desktop user menu on Linux/Windows: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2028473002/ BUG=615893 ========== to ========== Changed two icons for desktop user menu on Linux/Windows: 1. Changed the icon for the "Manage people" button; 2. Changed the icon for sync error on the titlebar. See design doc here: https://docs.google.com/document/d/17yHrKd_EU6WoIAgljnZmNCSJLRlbM3O2Pxr7aPEnq... This CL is dependent on https://codereview.chromium.org/2028473002/ BUG=615893 Committed: https://crrev.com/b877548e091c72b836324d65c4714e7461b53a45 Cr-Commit-Position: refs/heads/master@{#398141} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b877548e091c72b836324d65c4714e7461b53a45 Cr-Commit-Position: refs/heads/master@{#398141} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
