Add margin to right of tab group title #18203
Conversation
| @@ -17,6 +17,7 @@ | |||
| #undef TabGroupStyle | |||
|
|
|||
| const int TabGroupStyle::kStrokeThicknessForVerticalTabs = 4; | |||
| const int TabGroupStyle::kTitleAdjustment = 2; | |||
There was a problem hiding this comment.
As we included the original tab_group_style.cc, we might be able to access the original constant there. if so, we can remove this and use kTitleAdjustmentForEmptyHeader = 2; for consistency.
There was a problem hiding this comment.
Hi, Thanks for checking my PR! This was what I tried before but I did not figure our how to submit my code change in the original tab_group_style.cc. Do you have any clue how I can do that?
There was a problem hiding this comment.
I seem to be unclear about it. I think you got it right about modifying chromium_src/.../tab_group_style.cc . But I think you could do something like,
// chromium_src/.../tab_group_style.cc
int TabGroupStyle::GetTitleAdjustmentToTabGroupHeaderDesiredWidth(
return kTitleAdjustmentForEmptyHeader; // even this constant is defined in the origianl .cc file, we can use it here, I guess.
)
Then, you don't need to declare a new constant.
|
|
||
| int TabGroupStyle::GetTitleAdjustmentToTabGroupHeaderDesiredWidth( | ||
| const std::u16string title) const { | ||
| return kTitleAdjustment; |
There was a problem hiding this comment.
I think it'd be helpful if we have a comment about why we're overriding this, e.g.
"Sets spacing between title view and group header even when |title| is empty"
There was a problem hiding this comment.
And I'm curious how this will affect the vertical tab, or does nothing to it?
There was a problem hiding this comment.
It's not affecting the vertical tabs, and if we can modify the original constant then we don't need to override this anymore
|
Hi, I've run our CI bots with this PR and it seems that the PR isn't formatted properly. Did you happen to run |
I confirm that no security/privacy review is needed
Ticket for this issue is brave/brave-browser#29259
Description:
Before this PR, we only added a margin to empty title group headers, but non-empty headers also need some space between the header and its tabs.
Before:
After:
