Close tab page option #6570

NejcZdovc · 2017-01-07T10:26:44Z

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

Description

Resolves #5489

Auditors

@bbondy @bsclifton

Test Plan

open two tab pages
right click on one and click close tab page
all tabs in tab pages should be closed

bbondy · 2017-02-07T20:29:42Z

js/actions/windowActions.js

+   * @param {Object} framesList - List of frames
+   * @param {Object} framePropsList - List of frame properties to consider
+   */
+  closeTabPage: function (framesList, framePropsList) {


Could you name this tabPageClosed?

luixxiul · 2017-02-08T01:26:59Z

app/extensions/brave/locales/en-US/menu.properties

@@ -13,6 +13,7 @@ openLocation=Open Location…
 openSearch=Search for "{{selectedVariable}}"
 importFrom=Import from…
 closeWindow=Close Window
+closeTabPage=Close tab page


how about renaming this into tabPageClosed as well?

I was thinking about it, but I am not sure.

I think it's OK as-is; it matches the English and also consistent format with closeWindow

bbondy · 2017-03-08T15:37:58Z

js/actions/windowActions.js

+   * @param {Object[]} framePropsList - The properties of all frames to close
+   */
+  closeFrames: function (framePropsList) {
+    const activeFrameKey = windowStore.getState().get('activeFrameKey')


ideally there should be no logic inside the window action, I think you should just make it an easy api and have it declarative, so like windowAction.tabPageClosed(i) for the index to close. The store then does the work to figure out which frames it needs to close and closes them all in one swoop.

bsclifton · 2017-04-03T16:46:19Z

@NejcZdovc is this ready for review? if so, let's rebase 😄

NejcZdovc · 2017-04-03T20:06:29Z

This PR is done, tested and it's working correctly. The only thing is that there is some logic in window action, so it should be refactored according to @bbondy comment

bbondy · 2017-04-03T22:30:23Z

yes sorry, we can't accept new PRs with logic in the window action.

NejcZdovc · 2017-04-03T22:36:16Z

@bbondy totally understandable, will refactor it, not a problem

@bbondy

Resolves brave#5489 Auditors: @bbondy @bsclifton Test Plan: - open two tab pages - right click on one and click close tab page - all tabs in tab pages should be closed

@NejcZdovc

Auditors: @NejcZdovc

@bsclifton

Auditors: @bsclifton

NejcZdovc · 2017-04-05T00:46:50Z

@bbondy refactored, ready for a review

bsclifton · 2017-04-05T06:18:31Z

test/unit/state/frameStateUtilTest.js

@@ -283,4 +283,75 @@ describe('frameStateUtil', function () {
      })
    })
  })
+
+  describe('removeFrames', function () {


❤️ the tests

bsclifton

Looks great 😄 I had reviewed this with you earlier and definitely approve of the changes. ++!

cezaraugusto

Awesome work here. Feature works, tests pass and super cool the split of logic to a frameReducer. LGTM

cezaraugusto · 2017-04-05T16:20:34Z

app/renderer/reducers/frameReducer.js

+  return state
+}
+
+module.exports = frameReducer


awesome++++

luixxiul added PR/needs-QA-attention ☕ feature/tabsbar labels Jan 7, 2017

NejcZdovc self-assigned this Jan 9, 2017

NejcZdovc requested review from bbondy and bsclifton January 12, 2017 10:19

luixxiul added this to the 0.13.1 milestone Jan 12, 2017

luixxiul changed the base branch from master to 0.13.1-branch January 14, 2017 14:56

bsclifton force-pushed the 0.13.1-branch branch 5 times, most recently from 5098230 to 187fa9a Compare January 20, 2017 00:12

NejcZdovc changed the title ~~Added close tab page~~ Close tab page option Jan 22, 2017

bsclifton force-pushed the 0.13.1-branch branch 4 times, most recently from 65c7753 to 238cfd3 Compare January 25, 2017 19:19

bsclifton modified the milestones: 0.13.2, 0.13.1 Jan 26, 2017

bsclifton force-pushed the 0.13.1-branch branch from 67b2326 to 5eb09be Compare January 26, 2017 20:09

bsclifton closed this Jan 26, 2017

bsclifton changed the base branch from 0.13.1-branch to master January 26, 2017 21:18

bsclifton reopened this Jan 26, 2017

bbondy force-pushed the master branch from e7497e3 to c320129 Compare February 5, 2017 04:25

bbondy reviewed Feb 7, 2017

View reviewed changes

NejcZdovc force-pushed the feature/#5489-close-tab-page branch 3 times, most recently from b4277d1 to 9d79860 Compare February 7, 2017 21:30

luixxiul reviewed Feb 8, 2017

View reviewed changes

NejcZdovc mentioned this pull request Feb 15, 2017

Option to close an entire tabset #7264

Closed

NejcZdovc added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Feb 27, 2017

NejcZdovc force-pushed the feature/#5489-close-tab-page branch from 4be5b03 to 555eb87 Compare March 8, 2017 10:19

bbondy reviewed Mar 8, 2017

View reviewed changes

bsclifton modified the milestones: 0.13.7, 0.13.6 Mar 10, 2017

NejcZdovc added PR/work-in-progress ⚒ and removed PR/needs-QA-attention ☕ labels Mar 13, 2017

luixxiul added the PR/needs-rebase ۞ label Apr 3, 2017

Added close tab page

54cf311

Resolves brave#5489 Auditors: @bbondy @bsclifton Test Plan: - open two tab pages - right click on one and click close tab page - all tabs in tab pages should be closed

NejcZdovc removed the PR/needs-rebase ۞ label Apr 4, 2017

NejcZdovc force-pushed the feature/#5489-close-tab-page branch from 555eb87 to 54cf311 Compare April 4, 2017 20:20

bsclifton and others added 2 commits April 4, 2017 14:54

Creating new frameReducer to handle closing tab page

1c70de4

Auditors: @NejcZdovc

Moving close frame to new reducer

3b57a39

Auditors: @bsclifton

NejcZdovc requested a review from bbondy April 4, 2017 22:53

NejcZdovc added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Apr 4, 2017

bsclifton reviewed Apr 5, 2017

View reviewed changes

bsclifton approved these changes Apr 5, 2017

View reviewed changes

cezaraugusto approved these changes Apr 5, 2017

View reviewed changes

app/renderer/reducers/frameReducer.js

return state

}

module.exports = frameReducer

Copy link

Contributor

cezaraugusto Apr 5, 2017

Choose a reason for hiding this comment

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

awesome++++

cezaraugusto merged commit 387738a into brave:master Apr 5, 2017

bbondy mentioned this pull request Apr 5, 2017

Added context-menu option to close tabs in tab page #5489

Closed

luixxiul removed the request for review from bbondy April 5, 2017 17:21

cndouglas removed the PR/needs-QA-attention ☕ label Apr 10, 2017

NejcZdovc mentioned this pull request Jul 13, 2017

Close tab page is not working #9420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close tab page option #6570

Close tab page option #6570

NejcZdovc commented Jan 7, 2017 •

edited

Loading

bbondy Feb 7, 2017

NejcZdovc Feb 7, 2017

luixxiul Feb 8, 2017

NejcZdovc Feb 8, 2017

bsclifton Feb 8, 2017

luixxiul Feb 9, 2017

bbondy Mar 8, 2017

bsclifton Mar 10, 2017

bsclifton commented Apr 3, 2017 •

edited

Loading

NejcZdovc commented Apr 3, 2017

bbondy commented Apr 3, 2017

NejcZdovc commented Apr 3, 2017

NejcZdovc commented Apr 5, 2017

bsclifton Apr 5, 2017 •

edited

Loading

bsclifton left a comment

cezaraugusto left a comment

cezaraugusto Apr 5, 2017

Close tab page option #6570

Close tab page option #6570

Conversation

NejcZdovc commented Jan 7, 2017 • edited Loading

Description

Auditors

Test Plan

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

bsclifton commented Apr 3, 2017 • edited Loading

NejcZdovc commented Apr 3, 2017

bbondy commented Apr 3, 2017

NejcZdovc commented Apr 3, 2017

NejcZdovc commented Apr 5, 2017

bsclifton Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Jan 7, 2017 •

edited

Loading

bsclifton commented Apr 3, 2017 •

edited

Loading

bsclifton Apr 5, 2017 •

edited

Loading