Refactor of tabs related components (redux component) #8396

NejcZdovc · 2017-04-19T15:34:58Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Resolves #8690

Test Plan:

try reordering 4 tabs with dnd
try reordering 4 pinned tabs with dnd
should pass regular tab manual test https://github.com/brave/browser-laptop/wiki/Manual-Tests#tabs-pinning-and-tear-off-tabs

NejcZdovc · 2017-05-26T08:23:00Z

I will squash it after a review, don't merge without squashing it first

cezaraugusto

sequential tab closing isn't recovering tab size after mouse leaves the tab. see https://github.com/brave/browser-laptop/pull/8396/files#diff-15331dd5e010a9a3d250a1a37d851653R48 this is the only blocker I could find. There is also wrong variable name for audio muted, but just for proper readability, feature works.

cezaraugusto · 2017-05-27T02:41:34Z

app/renderer/components/tabs/content/audioTabIcon.js

-    // We switch to blue top bar for all breakpoints but default
-    return this.props.frame.get('breakpoint') === 'default'
+  get audioIcon () {
+    const isMuted = this.props.pageCanPlayAudio && !this.props.audioMuted


variable defines that page can play audio, and is not muted, maybe isNotMuted?

cezaraugusto · 2017-05-27T02:46:44Z

app/renderer/components/tabs/content/audioTabIcon.js

-  get pageCanPlayAudio () {
-    return !!this.props.frame.get('audioPlaybackActive')
+class AudioTabIcon extends React.Component {
+  constructor (props) {


is props required? maybe for redux component? afaik this is only needed if you want to use this.props in the constructor.

Yeah props is required because of redux. More about this PR #9061

cezaraugusto · 2017-05-27T02:47:57Z

app/renderer/components/tabs/content/audioTabIcon.js

+    const frame = frameStateUtil.getFrameByKey(currentWindow, ownProps.frameKey)
+
+    const props = {}
+    // used in renderer


can we remove this comment to avoid confusion?

Can you tell me a little bit more about this? I added this because this way you exactly know what is used in renderer and what in other functions in the component. Because of that changing and looking for thing should be easier then looking at a big list of variables.

I agree, but // used in renderer is followed by a blank line and then we have // used in other functions. If both applies wouldn't be better to // used in render and in other functions? Reading the code the blank line between both comments lead me to think that comment was left after some code removal.

Yeah sorry, didn't check out the code. If nothing is added for a renderer, this line should be removed. I agree

cezaraugusto · 2017-05-27T02:55:42Z

app/renderer/lib/tabUtil.js

@@ -32,85 +28,11 @@ module.exports.tabUpdateFrameRate = 66

 /**
 * Check whether or not current breakpoint match defined criteria
- * @param {Object} props - Object that hosts the tab breakpoint
+ * @param {Object} breakpoint - Brake point value


minor: typo

cezaraugusto · 2017-05-27T05:53:16Z

app/renderer/components/tabs/tabs.js

  }

  onMouseLeave () {
+    if (this.props.fixTabWidth) {
+      return


we are returning before fixTabWidth is set to null, this is making tabs unable to recover size after sequential tab close:

@cezaraugusto this if was copied from https://github.com/brave/browser-laptop/pull/8396/files#diff-15331dd5e010a9a3d250a1a37d851653L132. If I remove it from a function is working correctly, but I would like to understand why it was there in the first place

Ok, now I can see where the problem was. I switched if statement. It's fixed now.

cezaraugusto · 2017-05-27T06:01:31Z

fyi and QA guidance I manually tested the following, all passing.

✓ tabs breakpoint change based on window size with proper icons show/hide state
✓ privateTab icon and colors are properly shown
✓ audio indicator is properly shown and toggles on/off
✓ new session tabs shows partition number but not partition string
✓ tabs load with proper themecolor
✓ loading indicator shows properly when tab is loading
✓ closeTab icon closes a tab
✓ colored tabs shows white text when required
✓ colored tabs with white text should be bolder in macOS
✓ colored tabs with white text have a white shadow behind favicon
✓ websites without favicon show paper icon instead of icon
✓ reordering a tab with drag and drop works as expected
✓ caret for notification bar shows when notification is open
✓ Tab pages respect max tabs per page defined in preferences
✓ Tab pages switch back/forward actions
✓ Context menu for tabs display properly

NejcZdovc · 2017-05-27T07:59:37Z

@cezaraugusto thank you very much for a thorough review. I think that all you comments were addressed.

cezaraugusto

automated tests pass, str pass, own manual tests pass, comments addressed, nice work lgtm

bsclifton · 2017-06-02T07:03:31Z

possible small regression (something tests might help with):
Clicking the close button doesn't automatically highlight (turn orange) the next tabs close button

(it only turns orange when I click and then takes a moment to close because I have so many bookmarks)

bsclifton

While testing, I found some issues and fixed (see 446a7f2)

Changes look good! I manually tested (see feedback about close button not auto-highlighting). Other than that, I didn't find any issues 😄

(More tests are always welcome 😄 )

bsclifton · 2017-06-02T07:05:50Z

app/common/lib/faviconUtil.js

-      resolve(iconHref + resolution)
-    }
-  })
-}


Nice! Thanks for removing this 😄

Resolves brave#8690

@NejcZdovc

… way: - iconSize was not being exported correctly (it always had undefined). I moved to config and verified it has a value now - the tab size methods now handle null/undefined properly - updated tabContentState to use the braveExtensionId constant Auditors: @NejcZdovc

Refactor of tabs related components (redux component)

NejcZdovc added this to the 0.15.1 milestone Apr 19, 2017

NejcZdovc self-assigned this Apr 19, 2017

luixxiul added the QA/test-plan-required label Apr 19, 2017

bbondy force-pushed the master branch from f0df59d to 70e5d77 Compare April 23, 2017 04:21

NejcZdovc force-pushed the redux/tabs branch 3 times, most recently from 7e416ab to bc46948 Compare April 25, 2017 12:27

NejcZdovc mentioned this pull request Apr 27, 2017

Redux refactoring tracking issue #8528

Closed

51 tasks

NejcZdovc force-pushed the redux/tabs branch 5 times, most recently from ecde624 to e44e6d4 Compare April 28, 2017 20:53

NejcZdovc changed the title ~~refactor of TabsToolbar (redux component)~~ Refactor of tabs related components (redux component) Apr 28, 2017

bsclifton added the PR/work-in-progress ⚒ label Apr 29, 2017

NejcZdovc force-pushed the redux/tabs branch from e44e6d4 to 2c95da4 Compare May 1, 2017 09:02

cezaraugusto added the refactoring/redux-component label May 1, 2017

NejcZdovc force-pushed the redux/tabs branch 2 times, most recently from 01742ee to 1ecede5 Compare May 4, 2017 13:11

NejcZdovc modified the milestones: 0.15.4, 0.15.3 May 4, 2017

luixxiul added the PR/needs-rebase ۞ label May 6, 2017

NejcZdovc removed the PR/needs-rebase ۞ label May 19, 2017

NejcZdovc force-pushed the redux/tabs branch 7 times, most recently from c0adaaf to ad1decb Compare May 22, 2017 10:43

NejcZdovc requested review from bbondy, cezaraugusto, bsclifton and bridiver May 26, 2017 08:21

NejcZdovc mentioned this pull request May 26, 2017

Redux component - Tabs related components #8690

Closed

cezaraugusto suggested changes May 27, 2017

View reviewed changes

luixxiul added the PR/needs-rebase ۞ label May 27, 2017

NejcZdovc force-pushed the redux/tabs branch 2 times, most recently from 23252c5 to 84fd57a Compare May 27, 2017 07:57

NejcZdovc removed the PR/needs-rebase ۞ label May 27, 2017

cezaraugusto approved these changes May 27, 2017

View reviewed changes

NejcZdovc requested a review from luixxiul May 31, 2017 20:50

bsclifton force-pushed the redux/tabs branch from 84fd57a to 446a7f2 Compare June 2, 2017 08:24

bsclifton approved these changes Jun 2, 2017

View reviewed changes

app/common/lib/faviconUtil.js

resolve(iconHref + resolution)

}

})

}

Copy link

Member

bsclifton Jun 2, 2017

Choose a reason for hiding this comment

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

Nice! Thanks for removing this 😄

NejcZdovc added PR/ready-for-merge ✩✩✩ and removed PR/needs-QA-attention ☕ PR/ready-for-merge ✩✩✩ labels Jun 2, 2017

luixxiul added the PR/needs-rebase ۞ label Jun 2, 2017

NejcZdovc and others added 4 commits June 2, 2017 15:04

Converts Tabs related components into redux

0eac011

Resolves brave#8690

Moves functions into tabContentState

e53359c

Ref fixes

f4f9b70

bsclifton force-pushed the redux/tabs branch from 446a7f2 to 8c5e361 Compare June 2, 2017 22:05

bsclifton removed the PR/needs-rebase ۞ label Jun 2, 2017

bsclifton merged commit ea04da9 into brave:master Jun 2, 2017

bsclifton added a commit that referenced this pull request Jun 2, 2017

Merge pull request #8396 from NejcZdovc/redux/tabs

827a6fb

Refactor of tabs related components (redux component)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of tabs related components (redux component) #8396

Refactor of tabs related components (redux component) #8396

NejcZdovc commented Apr 19, 2017 •

edited

Loading

NejcZdovc commented May 26, 2017

cezaraugusto left a comment •

edited

Loading

cezaraugusto May 27, 2017

NejcZdovc May 27, 2017

cezaraugusto May 27, 2017

NejcZdovc May 27, 2017

cezaraugusto May 27, 2017

NejcZdovc May 27, 2017 •

edited

Loading

cezaraugusto May 27, 2017

NejcZdovc May 27, 2017

NejcZdovc May 27, 2017

cezaraugusto May 27, 2017

NejcZdovc May 27, 2017

cezaraugusto May 27, 2017

NejcZdovc May 27, 2017

NejcZdovc May 27, 2017

cezaraugusto commented May 27, 2017

NejcZdovc commented May 27, 2017

cezaraugusto left a comment

bsclifton commented Jun 2, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading

bsclifton Jun 2, 2017

Refactor of tabs related components (redux component) #8396

Refactor of tabs related components (redux component) #8396

Conversation

NejcZdovc commented Apr 19, 2017 • edited Loading

NejcZdovc commented May 26, 2017

cezaraugusto left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc May 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezaraugusto commented May 27, 2017

NejcZdovc commented May 27, 2017

cezaraugusto left a comment

Choose a reason for hiding this comment

bsclifton commented Jun 2, 2017 • edited Loading

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Apr 19, 2017 •

edited

Loading

cezaraugusto left a comment •

edited

Loading

NejcZdovc May 27, 2017 •

edited

Loading

bsclifton commented Jun 2, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading