Toolbar Styles #391
Merged
Toolbar Styles #391
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
petemill
force-pushed
the
ui/toolbar-styles
branch
from
902ead5
to
9688cd5
Compare
This was referenced Sep 4, 2018
|
@bbondy @simonhong This is ready to begin review. I've updated the description and created follow-up issues, but I think this is a nice start. I'm doing a final cross-platform manual test now too. |
petemill
force-pushed
the
ui/toolbar-styles
branch
from
9688cd5
to
ce91af0
Compare
simonhong
reviewed
Sep 5, 2018
|
|
||
| class BraveNewTabButton : public NewTabButton { | ||
| public: | ||
| using NewTabButton::NewTabButton; |
| #include "ui/views/view.h" | ||
| #include "ui/views/widget/widget_observer.h" | ||
|
|
||
| +class BraveNewTabButton; |
| @@ -6,15 +6,18 @@ | |||
|
|
|||
| #include <string> | |||
|
|
|||
|
|
|||
|
|
||
| } | ||
|
|
||
| const SkColor kPrivateLocationBarBackground = SkColorSetRGB(0x1b, 0x0e, 0x2c); |
There was a problem hiding this comment.
How about moving this constant to anonymous namespace?
| } | ||
|
|
||
| template <class T> | ||
| constexpr T DarkPrivateLight(OmniboxTint tint, |
There was a problem hiding this comment.
This template is needed? I found that only SkColor is used as template argument.
petemill
force-pushed
the
ui/toolbar-styles
branch
2 times, most recently
from
ff8f664
to
0208911
Compare
|
All feedback is now addressed - thanks @simonhong |
petemill
force-pushed
the
ui/toolbar-styles
branch
from
0208911
to
8b0e83f
Compare
petemill
force-pushed
the
ui/toolbar-styles
branch
from
8b0e83f
to
73706ff
Compare
Fallback dark and private themes to chromium incognito theme
Extend OmniboxTint to accept Private as well as Light and Dark
Note that the static LocationBarView::IsRounded is a misnomer since it controls function (i.e. "Should Omnibox Wrap LocationBar"), and not just the shape. Similarly, NewTabButton confusingly uses border radius to infer Button width, so some workarounds need to be employed to use content bounds width instead. Tab height is also reduced in order to match a smaller border radius, otherwise there is too much visual empty surface area.
petemill
force-pushed
the
ui/toolbar-styles
branch
from
73706ff
to
966b6d5
Compare
bbondy
approved these changes
Sep 5, 2018
NejcZdovc
approved these changes
Sep 5, 2018
|
0.55.x 2d32614 |
petemill
pushed a commit
to petemill/brave-core
that referenced
this pull request
Jul 5, 2019
update topsite tile click area
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix brave/brave-browser#670 (needs follow-up for light theme at brave/brave-browser#961)
Fix brave/brave-browser#785
Fix brave/brave-browser#787 (needs follow-up for bottom corner via brave/brave-browser#962)
Fix brave/brave-browser#786
Architecture:
layout_constantsoverrideomnibox_themeoverridePatching
Many things are tied to the static function
LocationBarView::IsRounded(). Unfortunately, we want this to returnfalsein some circumstances (e.g. drawing the border and the background), and not in others (e.g. drawing the omnibox popup).Therefore, this PR adds a new static function:
LocationBarView::ShouldOmniboxSurround().We can't use a chromium_src override for omnibox_popup_contents_view.cc to define
IsRoundedasShouldOmniboxSurround, since this includes the LocationBarView header file. We also need some, but not all calls from LocationBarView members to change which static function is called, so patching is the only method I've found so far...TODO
More color and shape refinements<-- Subsequent PRSubmitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist: