Issue 9306 related

[bridiver]

Submitter Checklist:
@cezaraugusto I did some related changes for #9306 that turned out not to be necessary, but are still good to have and can go into 0.19. Still need to check tests, but wanted to get this out there to look at

• 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).
• Tagged reviewers and labelled the pull request as needed.

Test Plan: #9542 (review)

1. Have 3 tabs open
2. Set the 1st tab as active one
3. Hover over the 2nd tab
4. Close it
5. Test that you do not see active tab content
6. Test that the 3rd tab is previewed

Reviewer Checklist:

Tests

• Adequate test coverage exists to prevent regressions
• Tests should be independent and work correctly when run individually or as a suite ref
• New files have MPL2 license header

[bsclifton]

When I wrote the unit tests for this, I was unable to find a way to hit this condition (falling back to first).

When you originally pulled the logic out of frameStateUtil and moved here, you had a similar block. Since it wasn't possible to hit, I had removed it. Did you see a case where this runs?

[bridiver]

This PR was changes I made that turned out not to be necessary for the fix and this is just leftover. It's actually not correct anyway because it should get the first tab in the window and that is already covered by "next" so this isn't needed

…

On Jun 17, 2017, at 11:13 PM, Brian Clifton ***@***.***> wrote: @bsclifton commented on this pull request. In app/browser/reducers/tabsReducer.js: > @@ -67,6 +67,11 @@ const updateActiveTab = (state, closeTabId) => { } } + // fall back to first tab + if (nextTabId === tabState.TAB_ID_NONE) { + nextTabId = tabState.get('tabs').first() + } + if (nextTabId !== tabState.TAB_ID_NONE) { When I wrote the unit tests for this, I was unable to find a way to hit this condition. When you originally pulled the logic out of frameStateUtil and moved here, you had a similar block. Since it wasn't possible to hit, I had removed it. Did you see a case where this runs? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

[cezaraugusto]

@bridiver are we following the dumb/smart components convention where util files are made to host the logic, leaving the main component just for presentational purposes?

I'm changing the way we preview tabs in #8852 and if this turns to be accepted first I'll need to re-work the logic

[bridiver]

the logic should be in the reducers (which can use state/util files) and yes, the components should be as "dumb" as possible. Ideally action should be things like tabClicked or tabMouseEnter letting the reducer to decide what actually needs to be done in response

[bridiver]

@cezaraugusto it looks like this overrides some recent changes you made to the component. Can you make sure this covers whatever changes you made?

[cezaraugusto]

@bridiver yes, thanks for the reminder. I'll make sure to update your changes.

#8852 will also change it so keeping an eye anyway on which goes first.

[bridiver]

I think #8852 is actually moving in the wrong direction with setPreviewMode. "setter" actions should be deprecated and we don't want to call multiple actions together. setPreviewMode doesn't provide any new information that setPreviewFrame doesn't already provide. I think better to merge this first (assuming there are no issues with it) and then make other changes

[bridiver]

there is an issue with this PR that I am fixing right now

[bridiver]

updated @cezaraugusto
I really think we should merge this one first because it's only going to get more complicated to merge the functionality if we keep adding things to the component

[cezaraugusto]

this regressed #9539 and also made next tab preview after close to flash with active tab.

for flashing of active tab content, here's the str:

1. Have three tabs open
2. Set first as active
3. Hover over second, close
4. Fourth tab is previewed as expected, but before preview happens you can see active tab content..

[cezaraugusto]

@bridiver btw once we're good to go I'm going to accept this one before the land of #8852 as per your request. That PR will be included in 0.18.x per last weekly triage meeting, so I'm going to move this to the same milestone as well.

[cezaraugusto]

I think those should be removed too

[cezaraugusto]

maybe remove it all given other handlers are not bound to the constructor but curious if that affects reduxComponent

[bridiver]

actually I'm surprised this doesn't cause an error, but I'll remove it

[cezaraugusto]

btw there's no listener for tab page previews anymore and I can't preview a tab set.

[bridiver]

ah, I think I accidentally grouped them together. Is there a test for it that is failing or did you just find it by manual testing?

[cezaraugusto]

yes, you can try npm run test -- --grep="tab pages tab page previews hovering over a tab page changes it"

[cezaraugusto]

Feature works fine, had a suspicious failling test with "tab tests webview previews when tab is hovered does not show tab previews when setting is off" but passing locally so intermittent. Thanks ++

[luixxiul]

@cezaraugusto I added the QA steps for the tab preview issue to the 1st post based on your feedback here: #9542 (review). Please correct them if they are not right (I replaced fourth tab with 3rd tab as there should be only 3 tabs opened).

[cezaraugusto]

@luixxiul awesome, thanks for that