Removes all non-primitive types from BookmarksToolbar and BookmarkTool… #9713

NejcZdovc · 2017-06-26T11:39:19Z

…barButton

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 #9712

Auditors: @bsclifton

Test plan:

Note: Reordering in context menu is not working. Will create issue for it.

right click on a blank bookmark toolbar
context menu should be displayed

right click on a bookmark
context menu should be displayed

right click on a bookmark folder
context menu should be displayed

left click on a bookmark folder
context menu should be displayed with items in the folder

create so many bookmark items that you get overflow menu
check that overflow menu is correctly displayed

check if bookmarks are displayed and updated correctly

DND test

try to reorder bookmarks on the bookmark toolbar
try to add bookmark into a folder
try to bookmark a tab with dragging lock icon on the bookmark toolbar

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

bridiver · 2017-06-26T18:52:01Z

app/common/lib/bookmarkUtil.js

    .filter((bookmark) => !bookmark.get('parentFolderId'))
+    .sort(siteUtil.siteSort)


++ on filter before sort

bridiver · 2017-06-26T19:30:51Z

app/common/lib/bookmarkUtil.js

@@ -119,9 +119,9 @@ const getToolbarBookmarks = (state) => {
  }

  return {
-    visibleBookmarks: noParentItems.take(i),
+    visibleBookmarks: noParentItems.take(i).map((item, index) => index).toList(),


not sure I understand this change

Instead of sending the whole object we are sending only keys. So we have list of keys and this way we are not triggering re-renders. We send this key to the child components which gets the whole object from the state.

I see, makes sense

bridiver · 2017-06-26T19:41:47Z

js/actions/bookmarkActions.js

 const {SWITCH_TO_NEW_TABS} = require('../constants/settings')
 const getSetting = require('../settings').getSetting

 const bookmarkActions = {
-  openBookmarksInFolder: function (allBookmarkItems, folderDetail) {
+  openBookmarksInFolder: function (folderDetail) {


this one should also be converted to an action. Not sure why this is called bookmarkActions, probably just some legacy reason because it's not an action the way we define them now

I think you can probably dump this whole file, make a real action and move the logic to the reducer

I agree that we need to convert this, but not sure if this PR is the right one to do it in. One thing that we also need to discuss is how can we call other actions inside the reducers. One case that I had was calling an actions that was then handled in multiple reducers. So we need to define this

we should not be calling actions from reducers, that breaks the uni-directional flow that is the whole point of flux/redux. The only time we are doing that now is for some special cases in the windowStore and maybe a few legacy calls. If you have a specific example to look at we can talk about options

as far as converting it goes, this is just like the others. It is accessing appStoreRenderer and that is problematic. Why is this any different than the others that you already converted?

I am not sure that that's enough. So if you check out createTabRequested you will see that we have two places where we call APP_CREATE_TAB_REQUESTED.

First: https://github.com/brave/browser-laptop/blob/master/app/browser/reducers/tabsReducer.js?utf8=%E2%9C%93#L138

Second: https://github.com/brave/browser-laptop/blob/master/app/browser/reducers/sitesReducer.js?utf8=%E2%9C%93#L158

Now if instead of calling this app actions I just do tabs.create some logic will be lost, which would be called otherwise. This is the main reason why I keep using actions, because they are used in different places and I could regress some logic if I just pick one. What I don't want to do is duplicating a code.

the handler in sitesReducer should be changed to APP_TAB_CREATED. APP_CREATE_TAB_REQUESTED is a temporary workaround until we move things to the browser process so nothing else should handle it

in any case if tabs.create doesn't encapsulate all the logic for creating a tab then we need to fix that because the api is not useful if it can only be called through APP_CREATE_TAB_REQUESTED

for instance activateIfOpen shouldn't be an action property because it always applies to specific urls

Based on our conversation yesterday I would suggest that we merged this PR. This file bookmarkActions.js was not really the focus in this PR, so I would suggest that we create a follow up where we would refactor only this file.

bridiver · 2017-06-26T19:47:20Z

app/renderer/components/bookmarks/bookmarkToolbarButton.js

+      e.stopPropagation()
+    }
+
+    windowActions.onShowBookmarkFolderMenu(this.props.bookmarkKey, left, top)


can you add a TODO to merge setBookmarksToolbarSelectedFolderId into onShowBookmarkFolderMenu. We should never call more than one action at a time

bridiver · 2017-06-26T19:49:06Z

app/renderer/reducers/contextMenuReducer.js

+      icon: showFavicon ? site.get('favicon') : undefined,
+      faIcon,
+      contextMenu: function (e) {
+        onSiteDetailMenu(state, item[0], e)


I think this is supposed to be windowActions. onSiteDetailMenu ?

is there a test that covers this? Looks like it should be failing if there is

NejcZdovc · 2017-07-07T08:27:24Z

re-run TEST_DIR=bookmark-components npm run testsuite 4 times locally and it's passing every single time

@bsclifton

…barButton Resolves brave#9712 Auditors: @bsclifton Test Plan:

ayumi · 2017-07-10T21:41:43Z

what's the status of this? currently UI lag is immense in master, encumbering other performance work. i'm really looking forward to this patch!

bridiver

++ with follow-up from action discussion

Removes all non-primitive types from BookmarksToolbar and BookmarkTool…

bsclifton

++

bsclifton · 2017-07-10T23:22:22Z

app/renderer/reducers/contextMenuReducer.js

@@ -89,6 +101,414 @@ const onTabPageMenu = function (state, action) {
  tabPageMenu.popup(getCurrentWindow())
 }

+const openInNewTabMenuItem = (url, isPrivate, partitionNumber, openerTabId) => {


Each of these menu types should be in a file (I'd suggest commonMenu- see https://github.com/brave/browser-laptop/blob/2bd7726b6d6ea83f41fbebdfab6b4141f8a5a78b/app/common/commonMenu.js). This would make it easy to unit test each method. A quick example of something easy to test: you can check each method that's exported to make sure each has a label field that called locale.translation().

Would be great in a follow up 😄 Everything else looks good

NejcZdovc added PR/work-in-progress ⚒ refactoring labels Jun 26, 2017

NejcZdovc added this to the 0.19.x (Nightly Channel) milestone Jun 26, 2017

NejcZdovc self-assigned this Jun 26, 2017

NejcZdovc added the perf label Jun 26, 2017

NejcZdovc force-pushed the refactor/#9712-bookmarks branch from 7178a74 to a873a16 Compare June 26, 2017 14:01

NejcZdovc changed the base branch from master to 0.18.x June 26, 2017 18:02

NejcZdovc changed the base branch from 0.18.x to master June 26, 2017 18:03

NejcZdovc changed the base branch from master to 0.18.x June 26, 2017 18:05

NejcZdovc changed the base branch from 0.18.x to master June 26, 2017 18:06

NejcZdovc force-pushed the refactor/#9712-bookmarks branch 4 times, most recently from e902032 to 147aba4 Compare June 26, 2017 18:21

NejcZdovc added refactoring/redux-component and removed refactoring labels Jun 26, 2017

NejcZdovc mentioned this pull request Jun 26, 2017

Redux refactoring tracking issue #8528

Closed

51 tasks

bridiver suggested changes Jun 26, 2017

View reviewed changes

bsclifton changed the title ~~Removes all non-primitve types from BookmarksToolbar and BookmarkTool…~~ Removes all non-primitive types from BookmarksToolbar and BookmarkTool… Jun 26, 2017

NejcZdovc force-pushed the refactor/#9712-bookmarks branch 2 times, most recently from 47f9dca to 642db43 Compare June 28, 2017 06:34

NejcZdovc removed the PR/work-in-progress ⚒ label Jun 28, 2017

NejcZdovc force-pushed the refactor/#9712-bookmarks branch from 642db43 to 8aff1dc Compare June 28, 2017 06:56

NejcZdovc requested review from bridiver, bsclifton and darkdh June 28, 2017 06:57

NejcZdovc force-pushed the refactor/#9712-bookmarks branch from 8aff1dc to 081ddc8 Compare June 28, 2017 07:16

NejcZdovc mentioned this pull request Jun 29, 2017

Only primitives in BookmarksToolbar and BookmarkToolbarButton #9712

Closed

NejcZdovc force-pushed the refactor/#9712-bookmarks branch from 081ddc8 to d49b5ca Compare June 30, 2017 11:25

NejcZdovc removed the request for review from bridiver July 5, 2017 16:26

NejcZdovc requested review from bridiver and bsclifton and removed request for bsclifton and darkdh July 5, 2017 16:26

NejcZdovc force-pushed the refactor/#9712-bookmarks branch from d49b5ca to 1466694 Compare July 5, 2017 19:10

NejcZdovc mentioned this pull request Jul 5, 2017

Tracking: Critical performance tracking issue #9311

Closed

35 tasks

NejcZdovc mentioned this pull request Jul 7, 2017

Refactor bookmarkActions #9907

Closed

NejcZdovc force-pushed the refactor/#9712-bookmarks branch 3 times, most recently from 9439534 to 3fa0af0 Compare July 9, 2017 20:15

Removes all non-primitve types from BookmarksToolbar and BookmarkTool…

88f5188

…barButton Resolves brave#9712 Auditors: @bsclifton Test Plan:

NejcZdovc force-pushed the refactor/#9712-bookmarks branch from 3fa0af0 to 88f5188 Compare July 10, 2017 06:09

NejcZdovc requested review from bridiver and bsclifton and removed request for bridiver and bsclifton July 10, 2017 06:34

bridiver approved these changes Jul 10, 2017

View reviewed changes

bbondy merged commit 03ae74b into brave:master Jul 10, 2017

bbondy added a commit that referenced this pull request Jul 10, 2017

Merge pull request #9713 from NejcZdovc/refactor/#9712-bookmarks

1c1e3fa

Removes all non-primitive types from BookmarksToolbar and BookmarkTool…

bsclifton reviewed Jul 10, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes all non-primitive types from BookmarksToolbar and BookmarkTool… #9713

Removes all non-primitive types from BookmarksToolbar and BookmarkTool… #9713

NejcZdovc commented Jun 26, 2017 •

edited

Loading

bridiver Jun 26, 2017

bridiver Jun 26, 2017

NejcZdovc Jun 26, 2017

bridiver Jun 26, 2017

bridiver Jun 26, 2017

bridiver Jun 26, 2017

NejcZdovc Jun 27, 2017

bridiver Jun 28, 2017

bridiver Jun 28, 2017

NejcZdovc Jul 6, 2017

bridiver Jul 6, 2017

bridiver Jul 6, 2017

bridiver Jul 6, 2017

NejcZdovc Jul 7, 2017

bridiver Jun 26, 2017

NejcZdovc Jun 27, 2017

bridiver Jun 26, 2017

bridiver Jun 26, 2017

NejcZdovc Jun 27, 2017

NejcZdovc commented Jul 7, 2017

ayumi commented Jul 10, 2017 •

edited

Loading

bridiver left a comment

bsclifton left a comment

bsclifton Jul 10, 2017

bsclifton Jul 10, 2017

		.filter((bookmark) => !bookmark.get('parentFolderId'))
		.sort(siteUtil.siteSort)

Removes all non-primitive types from BookmarksToolbar and BookmarkTool… #9713

Removes all non-primitive types from BookmarksToolbar and BookmarkTool… #9713

Conversation

NejcZdovc commented Jun 26, 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

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

Choose a reason for hiding this comment

NejcZdovc commented Jul 7, 2017

ayumi commented Jul 10, 2017 • edited Loading

bridiver left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Jun 26, 2017 •

edited

Loading

ayumi commented Jul 10, 2017 •

edited

Loading