This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
Convert tabs bar with Aphrodite #11530
Merged
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
Codecov Report
@@ Coverage Diff @@
## master #11530 +/- ##
==========================================
+ Coverage 52.65% 52.66% +0.01%
==========================================
Files 271 271
Lines 25715 25722 +7
Branches 4104 4104
==========================================
+ Hits 13539 13547 +8
+ Misses 12176 12175 -1
|
luixxiul
commented
Oct 15, 2017
| @@ -17,8 +17,7 @@ const frameStateUtil = require('../../../../../js/state/frameStateUtil') | |||
|
|
|||
| // Styles | |||
| const {theme} = require('../../styles/theme') | |||
| const {spacing, zindex} = require('../../styles/global') | |||
| const {opacityIncreaseKeyframes} = require('../../styles/animations') | |||
| const globalStyles = require('../../styles/global') | |||
There was a problem hiding this comment.
Before doing that task, let's open an issue to edit this section on docs/style.md to achieve a common understanding.
opened here -> #11540
luixxiul
commented
Oct 15, 2017
| boxSizing: 'border-box', | ||
| zIndex: zindex.zindexTabsThumbnail, | ||
| zIndex: globalStyles.zindex.zindexTabsThumbnail, |
8 tasks
luixxiul
commented
Oct 26, 2017
| @@ -22,7 +24,7 @@ class TabPages extends React.Component { | |||
| } | |||
|
|
|||
| render () { | |||
| return <div className='tabPageWrap'> | |||
| return <div className={css(styles.tabPageWrap)}> | |||
There was a problem hiding this comment.
It'll be renamed into tabPages (Wrap is not required with BEM naming)
luixxiul
added
PR/pending-review
QA/test-plan-required
QA/test-plan-specified
and removed
PR/needs-rebase ۞
QA/test-plan-required
labels
cezaraugusto
reviewed
Nov 3, 2017
| border: 'none', | ||
|
|
||
| ':hover': { | ||
| border: `1px solid ${theme.tabPage.active.hover.borderColor}` |
There was a problem hiding this comment.
active tab page should have the same state when hovering (filled orange)
There was a problem hiding this comment.
On theme.js, they are set like this:
tabPage: {
backgroundColor: '#fff',
borderColor: '#bbb',
hover: {
borderColor: globalStyles.color.braveOrange
},
active: {
backgroundColor: globalStyles.color.braveOrange,
hover: {
borderColor: globalStyles.color.braveOrange
}
}
So they should be same by default. Is it OK?
There was a problem hiding this comment.
not sure what is happening but this is what I'm seeing now:
There was a problem hiding this comment.
maybe you should enforce the background color on hover for the active tab page too
There was a problem hiding this comment.
Wow nice find! I addressed that here: 0582859#diff-b39b5cd9b6e26e313c7387108e17891cR198
Closes #11272 - Remove tabs.less - Set color properties on theme.js - Ser default properties on tabIcon.js - BrowserButton component is applied to back/forward button and menu button on the tabs bar Also: - Apply no-drag to the new tab button and tab area - Make the default icon large - Add will-change:opacity to the animations - Remove 'content' from theme.tab on theme.js Test Plan: 1. Test that tabs are pinnable 2. Test that tabs are unpinnable 3. Test that tabs are draggable to same tabset 4. Test that tabs are draggable to alternate tabset 5. Test that tabs can be teared off into a new window 6. Test that you are able to reattach a tab that is teared off into a new window 7. Test that tab pages can be closed 8. Test that tab pages can be muted Test Plan: 1. Test that the area around tab page indicators is draggable 2. Test that the gap between them is draggable
|
@cezaraugusto thanks for the review. I appreciate that very much ❤️ 😄 FYI: https://travis-ci.org/brave/browser-laptop/jobs/298379574 👍 |
8 tasks
cezaraugusto
pushed a commit
that referenced
this pull request
Nov 24, 2017
Also: - Remove bookmarksToolbar.less (it is no longer necessary after #11530; See 405b0a4#diff-474538b5e876b82157a7e1b6228743e8) - Specify the overflow indicator size Closes #12066 Auditors: @cezaraugusto Test Plan: 1. Add bookmarks as many as you see the overflow indicator on the bookmarks toolbar 2. Open about:preferences 3. Switch 'Bookmarks Bar' settings 4. Change the browser window's width 5. Make sure you see the overflow indicator on every setting
|
I'll have to move this milestone up to 0.22 (from 0.23) since #12565 is for 0.22 and is completely based on this refactor. Attempting merge to see if this is huge or not... |
petemill
pushed a commit
to petemill/browser-laptop
that referenced
this pull request
Mar 18, 2018
Convert tabs bar with Aphrodite Conflicts: app/renderer/components/styles/animations.js app/renderer/components/styles/global.js app/renderer/components/styles/theme.js app/renderer/components/tabs/tab.js
|
0.22.x 50f033f |
petemill
pushed a commit
that referenced
this pull request
Apr 10, 2018
Fix styling of bookmarksToolbar.js Also: - Remove bookmarksToolbar.less (it is no longer necessary after #11530; See 405b0a4#diff-474538b5e876b82157a7e1b6228743e8) - Specify the overflow indicator size Closes #12066 Auditors: @cezaraugusto Test Plan: 1. Add bookmarks as many as you see the overflow indicator on the bookmarks toolbar 2. Open about:preferences 3. Switch 'Bookmarks Bar' settings 4. Change the browser window's width 5. Make sure you see the overflow indicator on every setting
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Closes #11272
Also:
Test Plan:
CXX=g++-4.8 NODE_ENV=test TEST_DIR=tab-components npm run testsuiteFor QA team:
Test Plan:
Test Plan:
Submitter Checklist:
git rebase -ito squash commits (if needed).Reviewer Checklist:
Tests