Issue 10933049: Fixed position of plus image on action box button; added ICON_CENTERED placement to TextButton

Issue 10933049: Fixed position of plus image on action box button; added ICON_CENTERED placement to TextButton (Closed)

Created:
8 years, 3 months ago by yefimt

Modified:
8 years, 3 months ago

Reviewers:
Ben Goodger (Google)

CC:
chromium-reviews, tfarina

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Fixed position of plus image on action box button; added ICON_CENTERED placement to TextButton BUG=147330 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156599 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156824

Patch Set 3 : #

Created: 8 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+14 lines, -4 lines)			Patch
M	chrome/browser/ui/views/location_bar/action_box_button_view.cc	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/views/controls/button/text_button.h	View	1	1 chunk	+4 lines, -1 line	0 comments	Download
M	ui/views/controls/button/text_button.cc	View		2 chunks	+6 lines, -3 lines	0 comments	Download
M	ui/views/examples/button_example.cc	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Ben Goodger (Google)

lgtm http://codereview.chromium.org/10933049/diff/1/ui/views/controls/button/text_button.h File ui/views/controls/button/text_button.h (right): http://codereview.chromium.org/10933049/diff/1/ui/views/controls/button/text_button.h#newcode332 ui/views/controls/button/text_button.h:332: ICON_CENTERED // Centered make sense only when text ...

8 years, 3 months ago (2012-09-13 15:41:11 UTC) #2

yefimt

http://codereview.chromium.org/10933049/diff/1/ui/views/controls/button/text_button.h File ui/views/controls/button/text_button.h (right): http://codereview.chromium.org/10933049/diff/1/ui/views/controls/button/text_button.h#newcode332 ui/views/controls/button/text_button.h:332: ICON_CENTERED // Centered make sense only when text is ...

8 years, 3 months ago (2012-09-13 16:12:48 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10933049/1004

8 years, 3 months ago (2012-09-13 16:14:40 UTC) #4

Peter Kasting

Wait a minute, why are you using a TextButton if you have no text? Shouldn't ...

8 years, 3 months ago (2012-09-13 19:37:49 UTC) #6

yefimt

8 years, 3 months ago (2012-09-13 19:40:13 UTC) #7

On 2012/09/13 19:37:49, Peter Kasting wrote:
> Wait a minute, why are you using a TextButton if you have no text?  Shouldn't
> you be using an ImageButton instead?
> 
> Please explain why TextButton is really the right choice here or else revert
> this change and switch the action button to using an ImageButton.

ActionButton is derived from MenuButton, which derived from TextButton
So my options were to add centered to text button or implement MenuImageButton.
First option looks better because it doesnt create code duplication.

Expand Messages | Collapse Messages