-
Notifications
You must be signed in to change notification settings - Fork 867
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
Increase contrast between active and inactive tabs #6900
Conversation
If mouse is hovered on tab, shows close button.
* light theme #BEBEBF * dark theme #393838 * no color changes at this time for private window / with tor tab separator and custom theme
1af4f29
to
6ead25d
Compare
@@ -1,16 +1,48 @@ | |||
diff --git a/chrome/browser/ui/views/tabs/tab_style_views.cc b/chrome/browser/ui/views/tabs/tab_style_views.cc | |||
index 950c967b5f87a6e3f531e3155a420002a7510d3a..a9b5b74a1a5c345727c1deb631fce7fcbb2762dc 100644 | |||
index 950c967b5f87a6e3f531e3155a420002a7510d3a..ad84681855bc39f55796c8740f490b998e2bc68a 100644 |
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't avoid patching. GM2TabStyle
is defined in anonymous namespace.
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.
@simonhong: wasn't sure this was going to work, so had to try it out and it seems to be working. What do you think of subclassing instead like this: 4c19f3e
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 @mkarolin ! Overriding CreateForTab
is great :)
Updated.
* Inactive tab title has 70% opacity; on hover or focus show title at 100% opacity * Make active tab font Semibold
6ead25d
to
6cf3dd7
Compare
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.
Looks great! 💯
Do tab separators seem a bit lost now? @karenkliu |
@mkarolin Yep - this is the intended design. The favicon + title of inactive tabs already do a lot to differentiate between tabs, so the tab separator contrast was reduced. This will also help the active tab stand out more within the tabbar. |
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.
LGTM
@mkarolin Thanks for great review 👍 |
Resolves brave/brave-browser#8576
Submitter Checklist:
npm run lint
,npm run gn_check
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).QA/Yes
orQA/No
) to the associated issuerelease-notes/include
orrelease-notes/exclude
) to the associated issueTest Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.