Refactors TabPage and TabPages into redux #9414
Conversation
There was a problem hiding this comment.
Likely not caused by this PR, but I did discover a bug.
- Have two tab pages
- In tab page 1, mute one of the two videos
- Switch to tab page 2
- Right click tab page indicator, pick "Mute Tabs"
- Notice that the mute for tabs is toggled, not set to false
- Picking unmute seems to work fine
update: captured with #9464
| const sourceDragData = dndData.getDragData(e.dataTransfer, dragTypes.TAB) | ||
| const sourceDragFromPageIndex = this.props.sourceDragFromPageIndex | ||
| // This must be executed async because the state change that this causes | ||
| // will cause the onDragEnd to never run | ||
| setTimeout(() => { | ||
| // If we're moving to a right page, then we're already shifting everything to the left by one, so we want | ||
| // to drop it on the right. | ||
| windowActions.moveTab(sourceDragData.get('key'), moveToFrame.get('key'), | ||
| windowActions.moveTab(sourceDragData.get('key'), this.props.moveToFrameKey, |
There was a problem hiding this comment.
Should we still be using windowActions / frameKey? I think it would make more sense to use appAction / tabId
There was a problem hiding this comment.
Although, that change might be quite a bit larger. I'd be OK with us doing that later 😄
There was a problem hiding this comment.
Yeah when I am doing refactors I try to keep it as close as possible, because this way we are less error prone and we will have a few more iterations of redux components anyway when we will start moving into apps state (state helpers)
| const tabsPerPage = Number(getSetting(settings.TABS_PER_PAGE)) | ||
|
|
||
| const props = {} | ||
| props.tabPageCount = Math.ceil(frames.size / tabsPerPage) |
There was a problem hiding this comment.
can this but put into a property? that way we can unit test it 😄
There was a problem hiding this comment.
You mean into function?
There was a problem hiding this comment.
@NejcZdovc yeah- basically the ES6 classes have "properties" when they're exposed using a getter. Like:
class TabPage extends React.Component {
//..
get tabPageCount () {
return Math.ceil(this.frames.size / this.tabsPerPage)
}
//..versus a typical method or function
class TabPage extends React.Component {
//..
tabPageCount () {}
}
function tabPageCount() {}There was a problem hiding this comment.
Getters are very easy to test using Enzyme 😄
There was a problem hiding this comment.
Problem is that you can't use getter in mergeProps, because you don't have access to the component. So what we can do is create helper in util file
There was a problem hiding this comment.
ahh that's right! I had forgotten 😄 I saw you use getters in #9413, but I now see those are in render. Util function would be great 😄
There was a problem hiding this comment.
Yes you can use getters for html content so that renderer is not that big, but one thing that you need to be carful is that you always trigger re-render when needed. That means that you shouldn't do any calculation inside the getter that can change getter return.
There was a problem hiding this comment.
Moved into util file and added unit tests
Resolves brave#9339 Auditors: @bsclifton @bridiver Test Plan: - try to open enaugh tabs to trigger tab page (default is 20) - see if audio indicator is displayed if you have video playing in tab page - check if you can close tab page
Test Plan
Description
Submitter Checklist:
git rebase -ito squash commits (if needed).Resolves #9339
Auditors: @bsclifton @bridiver
Reviewer Checklist: