Skip to content
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

Merged
merged 3 commits into from
Jan 20, 2018
Merged

rectangle tab shape #22

merged 3 commits into from
Jan 20, 2018

Conversation

AlexeyBarabash
Copy link
Contributor

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.

screenshot from 2018-01-17 16-26-19

Corresponding branch of Brave/Brave with antimuon reference is https://github.com/brave/brave/tree/customize_tab_shape .

return hybrid ? 3 : 1;
case LOCATION_BAR_HEIGHT:
return hybrid ? 32 : 28;
case TABSTRIP_NEW_TAB_BUTTON_OVERLAP:
Copy link
Collaborator

@bridiver bridiver Jan 17, 2018

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?

Copy link
Contributor Author

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

@bridiver
Copy link
Collaborator

why are we making the new tab button rectangular? It's square in the current version of Brave

@bridiver
Copy link
Collaborator

bridiver commented Jan 18, 2018

I still see the same tab shape on macos after making these changes. Am I missing something?

@bridiver
Copy link
Collaborator

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

@bridiver
Copy link
Collaborator

oh, I know what the issue is. This is only the UI views side (windows/linux) and not macos

ui::MaterialDesignController::MATERIAL_HYBRID;
switch (inset) {
case TAB:
// Originally here was `return gfx::Insets(1, hybrid ? 18 : 16)`
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

  1. Original tab
  2. Rectangle tab with inset 16
  3. 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.

tab insets4

// DIP.

// This is zero for rectangle shaped tab
return 0;
Copy link
Collaborator

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

Copy link
Contributor Author

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
image

but idea do not touch internals of this method is good, I will look, maybe it is possible to achieve it

Copy link
Contributor Author

@AlexeyBarabash AlexeyBarabash Jan 18, 2018

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.

@bridiver
Copy link
Collaborator

bridiver commented Jan 18, 2018

so, it seems like we can accomplish this by returning Tab::kMinimumEndcapWidth from GetTabEndcapWidth and returning 0 from TABSTRIP_NEW_TAB_BUTTON_OVERLAP (like you have now). In this case the smallest patch would just patch those directly. If we want to make the newtab button square we could also patch that to return hybrid ? gfx::Size(21, 21) : gfx::Size(18, 18);

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@AlexeyBarabash
Copy link
Contributor Author

AlexeyBarabash commented Jan 18, 2018

why are we making the new tab button rectangular? It's square in the current version of Brave

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.

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

Thanks, will test and fix.

about Mac OS

Will try to build and run it.

@AlexeyBarabash
Copy link
Contributor Author

Made changes according to the review.
@bridiver , the approach pointed in #22 (comment) works perfect.

image

Configuring build of MacOS .

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ thanks!

@bbondy bbondy merged commit 64f3a3c into master Jan 20, 2018
@AlexeyBarabash
Copy link
Contributor Author

@jonathansampson , could you please check how this works on high-dpi Linux and Windows devices?

@jonathansampson
Copy link
Contributor

@AlexeyBarabash I'm happy to help; I've never built this project though. Can you provide a Windows x64 exe for me to check?

@bridiver
Copy link
Collaborator

bridiver commented Jan 27, 2018 via email

@AlexeyBarabash
Copy link
Contributor Author

@jonathansampson I will back to you once I will build it, never done that before.

@bsclifton bsclifton deleted the customize_tab_shape branch June 18, 2018 06:26
NejcZdovc pushed a commit that referenced this pull request Dec 10, 2018
bbondy added a commit that referenced this pull request Feb 18, 2019
allow brave-extension to run on chrome
fmarier pushed a commit that referenced this pull request Oct 29, 2019
update to chromium 61.0.3163.79
antonok-edm added a commit that referenced this pull request Mar 10, 2021
update adblock-rust to v0.3 API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants