Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711443003:
[Mac] Adjustments to the Default State Touch Bar (Closed)
Description[Mac] Adjustments to the Default State Touch Bar
- Modified the text on the search button
- Use Google Search icon when the default search provider is Google
- New Tab button moved to the back
- Active star color should be blue
BUG=694754, 694760, 694756, 694753
Review-Url: https://codereview.chromium.org/2711443003
Cr-Commit-Position: refs/heads/master@{#452874}
Committed: https://chromium.googlesource.com/chromium/src/+/c73de4e7f56bfa4ece4ebb69086861ae40f17564
Patch Set 1 #Patch Set 2 : Nit #Patch Set 3 : Fixed star color #
Total comments: 10
Patch Set 4 : Fix for rsesek and estade #
Messages
Total messages: 27 (18 generated)
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Mac] Adjustments to the Default State Touch Bar - Modified the text on the search button - Use Google Search icon when the default search provider is Google - New Tab button moved to the back BUG=694754, 694760, 694756 ========== to ========== [Mac] Adjustments to the Default State Touch Bar - Modified the text on the search button - Use Google Search icon when the default search provider is Google - New Tab button moved to the back BUG=694754, 694760, 694756 ==========
spqchan@chromium.org changed reviewers: + estade@chromium.org, rsesek@chromium.org
Description was changed from ========== [Mac] Adjustments to the Default State Touch Bar - Modified the text on the search button - Use Google Search icon when the default search provider is Google - New Tab button moved to the back BUG=694754, 694760, 694756 ========== to ========== [Mac] Adjustments to the Default State Touch Bar - Modified the text on the search button - Use Google Search icon when the default search provider is Google - New Tab button moved to the back - Active star color should be blue BUG=694754, 694760, 694756, 694753 ==========
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL rsesek for cocoa estade for ui/gfx
Mostly lg https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (left): https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:152: isStarred_ ? kStarredIconColor : kTouchBarDefaultIconColor; Why this change? https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:197: kTouchBarIconSize, kTouchBarDefaultIconColor), Nit: this color is ignored. To make that more clear I'd use gfx::kPlaceholderColor https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... File ui/gfx/vector_icons/google_search_mac_touchbar.icon (right): https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... ui/gfx/vector_icons/google_search_mac_touchbar.icon:5: CANVAS_DIMENSIONS, 60, Scaling down from 60 to 32 (16?) may not look amazing as they don't have a very large GCD (4). Are designers happy with the appearance?
LGTM w/ a nit https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:182: const TemplateURL* default_provider = naming: defaultProvider
Patchset #4 (id:60001) has been deleted
PTAL https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (left): https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:152: isStarred_ ? kStarredIconColor : kTouchBarDefaultIconColor; On 2017/02/22 16:14:24, Evan Stade wrote: > Why this change? The resulting color is gray, not blue. One thing I can do instead is to change the value of kColorId_ProminentButtonColor. I'm not sure if I'll break anything though. https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:182: const TemplateURL* default_provider = On 2017/02/22 16:48:55, Robert Sesek wrote: > naming: defaultProvider Done. https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:197: kTouchBarIconSize, kTouchBarDefaultIconColor), On 2017/02/22 16:14:24, Evan Stade wrote: > Nit: this color is ignored. To make that more clear I'd use > gfx::kPlaceholderColor Done. https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... File ui/gfx/vector_icons/google_search_mac_touchbar.icon (right): https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... ui/gfx/vector_icons/google_search_mac_touchbar.icon:5: CANVAS_DIMENSIONS, 60, On 2017/02/22 16:14:24, Evan Stade wrote: > Scaling down from 60 to 32 (16?) may not look amazing as they don't have a very > large GCD (4). Are designers happy with the appearance? That's a good point. It looks fine on the touch bar and the designers are happy with it. I modified it to 32x32 just in case though.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/22 19:07:53, spqchan wrote: > PTAL > > https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (left): > > https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_touch_bar.mm:152: isStarred_ ? > kStarredIconColor : kTouchBarDefaultIconColor; > On 2017/02/22 16:14:24, Evan Stade wrote: > > Why this change? > > The resulting color is gray, not blue. One thing I can do instead is to change > the value of kColorId_ProminentButtonColor. I'm not sure if I'll break anything > though. > > https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): > > https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_touch_bar.mm:182: const TemplateURL* > default_provider = > On 2017/02/22 16:48:55, Robert Sesek wrote: > > naming: defaultProvider > > Done. > > https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_touch_bar.mm:197: kTouchBarIconSize, > kTouchBarDefaultIconColor), > On 2017/02/22 16:14:24, Evan Stade wrote: > > Nit: this color is ignored. To make that more clear I'd use > > gfx::kPlaceholderColor > > Done. > > https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... > File ui/gfx/vector_icons/google_search_mac_touchbar.icon (right): > > https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... > ui/gfx/vector_icons/google_search_mac_touchbar.icon:5: CANVAS_DIMENSIONS, 60, > On 2017/02/22 16:14:24, Evan Stade wrote: > > Scaling down from 60 to 32 (16?) may not look amazing as they don't have a > very > > large GCD (4). Are designers happy with the appearance? > > That's a good point. It looks fine on the touch bar and the designers are happy > with it. I modified it to 32x32 just in case though. ping :)
lgtm https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (left): https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:152: isStarred_ ? kStarredIconColor : kTouchBarDefaultIconColor; On 2017/02/22 19:07:50, spqchan wrote: > On 2017/02/22 16:14:24, Evan Stade wrote: > > Why this change? > > The resulting color is gray, not blue. One thing I can do instead is to change > the value of kColorId_ProminentButtonColor. I'm not sure if I'll break anything > though. huh, well that's weird. I guess on mac the bookmark star hardcodes it too: https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/location_bar/sta... I would think we want this color to be in native theme but I guess I don't mind if cocoa just hardcodes it in a bunch of files. https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... File ui/gfx/vector_icons/google_search_mac_touchbar.icon (right): https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... ui/gfx/vector_icons/google_search_mac_touchbar.icon:5: CANVAS_DIMENSIONS, 60, On 2017/02/22 19:07:52, spqchan wrote: > On 2017/02/22 16:14:24, Evan Stade wrote: > > Scaling down from 60 to 32 (16?) may not look amazing as they don't have a > very > > large GCD (4). Are designers happy with the appearance? > > That's a good point. It looks fine on the touch bar and the designers are happy > with it. I modified it to 32x32 just in case though. it looks like you just scaled down the svg to 32 before converting to a .icon, which accomplishes the same thing as if you left it at 60.
On 2017/02/24 07:22:15, Evan Stade wrote: > lgtm > > https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (left): > > https://codereview.chromium.org/2711443003/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_touch_bar.mm:152: isStarred_ ? > kStarredIconColor : kTouchBarDefaultIconColor; > On 2017/02/22 19:07:50, spqchan wrote: > > On 2017/02/22 16:14:24, Evan Stade wrote: > > > Why this change? > > > > The resulting color is gray, not blue. One thing I can do instead is to change > > the value of kColorId_ProminentButtonColor. I'm not sure if I'll break > anything > > though. > > huh, well that's weird. I guess on mac the bookmark star hardcodes it too: > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/location_bar/sta... > > I would think we want this color to be in native theme but I guess I don't mind > if cocoa just hardcodes it in a bunch of files. > > https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... > File ui/gfx/vector_icons/google_search_mac_touchbar.icon (right): > > https://codereview.chromium.org/2711443003/diff/40001/ui/gfx/vector_icons/goo... > ui/gfx/vector_icons/google_search_mac_touchbar.icon:5: CANVAS_DIMENSIONS, 60, > On 2017/02/22 19:07:52, spqchan wrote: > > On 2017/02/22 16:14:24, Evan Stade wrote: > > > Scaling down from 60 to 32 (16?) may not look amazing as they don't have a > > very > > > large GCD (4). Are designers happy with the appearance? > > > > That's a good point. It looks fine on the touch bar and the designers are > happy > > with it. I modified it to 32x32 just in case though. > > it looks like you just scaled down the svg to 32 before converting to a .icon, > which accomplishes the same thing as if you left it at 60. Thanks, everyone
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2711443003/#ps80001 (title: "Fix for rsesek and estade")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487955721846150,
"parent_rev": "a762bcb3f29e457e0915c501799c166487113256", "commit_rev":
"c73de4e7f56bfa4ece4ebb69086861ae40f17564"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Adjustments to the Default State Touch Bar - Modified the text on the search button - Use Google Search icon when the default search provider is Google - New Tab button moved to the back - Active star color should be blue BUG=694754, 694760, 694756, 694753 ========== to ========== [Mac] Adjustments to the Default State Touch Bar - Modified the text on the search button - Use Google Search icon when the default search provider is Google - New Tab button moved to the back - Active star color should be blue BUG=694754, 694760, 694756, 694753 Review-Url: https://codereview.chromium.org/2711443003 Cr-Commit-Position: refs/heads/master@{#452874} Committed: https://chromium.googlesource.com/chromium/src/+/c73de4e7f56bfa4ece4ebb690868... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/c73de4e7f56bfa4ece4ebb690868... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
