-
Notifications
You must be signed in to change notification settings - Fork 920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rectangle tab shape #22
Conversation
browser/ui/brave_layout_constants.cc
Outdated
return hybrid ? 3 : 1; | ||
case LOCATION_BAR_HEIGHT: | ||
return hybrid ? 32 : 28; | ||
case TABSTRIP_NEW_TAB_BUTTON_OVERLAP: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just adjust NEW_TAB_BUTTON
width to account for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, no.
this value is like -dx offset for new tab button relatively to right border of the most right tab.
Modifying NEW_TAB_BUTTON
with will not have effect on relative position of the right tab and NEW_TAB_BUTTON
why are we making the new tab button rectangular? It's square in the current version of Brave |
I still see the same tab shape on macos after making these changes. Am I missing something? |
shouldn't there be a new icon to go with this? In either case the hit test boundaries appear to be unaffected and still match up to the trapezoidal tabs |
oh, I know what the issue is. This is only the UI views side (windows/linux) and not macos |
browser/ui/brave_layout_constants.cc
Outdated
ui::MaterialDesignController::MATERIAL_HYBRID; | ||
switch (inset) { | ||
case TAB: | ||
// Originally here was `return gfx::Insets(1, hybrid ? 18 : 16)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also used in a few other places so maybe we shouldn't change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value describes the distance between outer border and inner content border of element. Here is a screenshot, marked it with a green line.
- Original tab
- Rectangle tab with inset 16
- Rectangle tab with inset 8
In other cases it is used for calculation of space available for text/icons inside of tab. I think it is safe to change it.
// DIP. | ||
|
||
// This is zero for rectangle shaped tab | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since GetTabEndcapWidth
== 4 and kMinimumEndcapWidth
== 4 it doesn't seem like we need to change this since it will already be 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kMinimumEndcapWidth == 2
,
https://github.com/brave/antimuon/pull/22/files/ab6ae3d7344c537faade6d01985c9a088242fc1a#diff-4933cc70f0015fa95af77b7a5763b5e1R14
but idea do not touch internals of this method is good, I will look, maybe it is possible to achieve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it. kMinimumEndcapWidth=4
should be unchanged. Then tab.h
and GetInverseDiagonalSlope
are unchanged.
so, it seems like we can accomplish this by returning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments
I was concentrated on tab shape. When I have made it, I modified inclined new tab button to a rectangle and stopped. Will make it square and the same style as current desktop Brave has.
Thanks, will test and fix.
Will try to build and run it. |
Made changes according to the review. Configuring build of MacOS . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ thanks!
@jonathansampson , could you please check how this works on high-dpi Linux and Windows devices? |
@AlexeyBarabash I'm happy to help; I've never built this project though. Can you provide a Windows x64 exe for me to check? |
Build process is identical to blb
… On Jan 26, 2018, at 9:37 PM, Sampson ***@***.***> wrote:
@AlexeyBarabash I'm happy to help; I've never built this project though. Can you provide a Windows x64 exe for me to check?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@jonathansampson I will back to you once I will build it, never done that before. |
allow brave-extension to run on chrome
update to chromium 61.0.3163.79
update adblock-rust to v0.3 API
This is PR for brave/brave-browser-snap#21 .
Approach used here - to tune tab parameters to make a trapezoidal tab to be rectangular. Also new tab button has been made rectangular.
Corresponding branch of Brave/Brave with antimuon reference is https://github.com/brave/brave/tree/customize_tab_shape .