Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fixes close tabs to the left/right #9790

Merged
merged 1 commit into from
Jul 14, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jun 30, 2017

when you have multiple tab pages

Submitter Checklist:

  • 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.

Resolves #9789

Auditors: @bsclifton

Test Plan:

  • open so many tabs that you have at least two tab pages
  • right click on the last tab
  • select "Close tabs to the left"

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

@NejcZdovc NejcZdovc added this to the 0.19.x (Nightly Channel) milestone Jun 30, 2017
@NejcZdovc NejcZdovc self-assigned this Jun 30, 2017
@NejcZdovc
Copy link
Contributor Author

PR blocked by #9773

@NejcZdovc NejcZdovc force-pushed the hotfix/#9789-tabpage branch from bcda03f to ba0ca31 Compare June 30, 2017 11:26
@NejcZdovc NejcZdovc modified the milestones: 0.18.x (Beta Channel), 0.19.x (Developer Channel) Jul 11, 2017
@NejcZdovc NejcZdovc force-pushed the hotfix/#9789-tabpage branch from ba0ca31 to 84aee6e Compare July 12, 2017 11:27
@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jul 12, 2017

This is done, but test will fail, because of this bug #9420 Not the right place, something else is wrong

@NejcZdovc NejcZdovc force-pushed the hotfix/#9789-tabpage branch 2 times, most recently from 5d0301d to 0f509d4 Compare July 13, 2017 19:25
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

The new test is failing locally when run individually and as part of a suite. Also travis shows the test as failing.

{
const currentIndex = frameStateUtil.getIndexByTabId(state, action.tabId)
if (currentIndex === -1) {
return
Copy link
Member

Choose a reason for hiding this comment

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

I know it was like this before as well but this should be returning state here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@bbondy
Copy link
Member

bbondy commented Jul 14, 2017

Ideally this whole thing would happen in the browser side instead but I know this is just a small fix to an existing bug so it's ok in general. But longer term we should refactor it. Test problem and returning state are blockers though.

@NejcZdovc NejcZdovc force-pushed the hotfix/#9789-tabpage branch from 0f509d4 to 96d3a14 Compare July 14, 2017 06:38
@NejcZdovc
Copy link
Contributor Author

@bbondy Test fixed as well. Should be good to go now

@NejcZdovc NejcZdovc force-pushed the hotfix/#9789-tabpage branch from 96d3a14 to a47b252 Compare July 14, 2017 06:39
@NejcZdovc NejcZdovc requested a review from bbondy July 14, 2017 06:39
when you have multiple tab pages

Resolves brave#9789

Auditors: @bsclifton

Test Plan:
@NejcZdovc NejcZdovc force-pushed the hotfix/#9789-tabpage branch from a47b252 to 4342b8c Compare July 14, 2017 07:48
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

test plan works, automated tests pass, lgtm

@bbondy bbondy merged commit 8b27a11 into brave:master Jul 14, 2017
bbondy added a commit that referenced this pull request Jul 14, 2017
@bbondy bbondy modified the milestones: 0.19.x (Beta Channel), 0.18.x (Release Channel) Jul 14, 2017
@bbondy
Copy link
Member

bbondy commented Jul 14, 2017

Doesn't apply without redux to 0.18.x so we're moving this to 0.19.x

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants