Bookmarks ordering #204

SergeyZhukovsky · 2018-05-11T15:05:25Z

No description provided.


        Add order field and commands


        Handle order commands


        Travis fixes

diracdeltas · 2018-05-11T19:36:36Z

client/constants/messages.js

+  DELETE_SYNC_SITE_SETTINGS: _,
+  /**
+   * browser -> webview
+   * browser sends this to get base bookmarks order for the particular device.


does this also need to send the device type? (Mobile vs Desktop)
@davidtemkin's spec says "Desktop devices come first, followed by mobile devices."

NVM, i see it's the platform string.

diracdeltas · 2018-05-11T19:39:55Z

yarn.lock

@@ -0,0 +1,4936 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


please delete this file since this repo no longer uses Yarn (has been replaced by package-lock.json)

diracdeltas

Could you please split getBaseBookmarksOrder and getBookmarkOrder into a utility module (like client/bookmarkUtil.js) and then add some unit tests for this module (liketest/client/bookmarkUtil.js) ?

diracdeltas · 2018-05-11T19:45:58Z

client/sync.js

+      platformPrefix = `2.`
+    }
+    platformPrefix += deviceId + `.`
+    return platformPrefix


minor: this function could be simplified into

return `${(platform === 'ios' || platform === 'android') ? 2 : 1}.${deviceId}.`

diracdeltas · 2018-05-11T19:49:03Z

client/sync.js

+  }
+
+  const getBookmarkOrder = (prevOrder, nextOrder) => {
+    let prevOrderSplit = prevOrder.split(`.`)


you can replace the backtick quote with single quotes (') for strings in JS in most of this code since backticks are only needed for template strings. by convention, if a string is not a template string, it uses quotes instead of backticks. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

diracdeltas · 2018-05-11T19:51:20Z

client/sync.js

+    return platformPrefix
+  }
+
+  const getBookmarkOrder = (prevOrder, nextOrder) => {


this should handle the case where prevOrder or nextOrder are null/empty (in order to get a bookmark at the start of the list or at the end of the list) - see comment in https://docs.google.com/a/brave.com/document/d/1t2IEFQF0Xj2aNnlEf0KaIXEqbfQ_RCn4b01LuIuef6I/edit?disco=AAAABxDSFGk

^ nvm, i see you assume they are empty strings instead of null. 👍


        Unit tests, small fixes


        Travis fixes


        Add more tests and handle orders ending with 0


        Merge remote-tracking branch 'origin/staging' into bookmarks_ordering


        Bookmarks order fix

diracdeltas · 2018-05-15T06:05:36Z

client/bookmarkUtil.js

@@ -60,7 +60,7 @@ module.exports.getBookmarkOrder = (prevOrder, nextOrder) => {
        order += prevOrderSplit[i] + '.'
      }
      let lastNumber = parseInt(prevOrderSplit[prevOrderSplit.length - 1])
-      order = getNextOrderFromPrevOrder(lastNumber, order)
+      order = getNextOrderFromPrevOrder(lastNumber,  order)


extra space

diracdeltas · 2018-05-15T06:10:11Z

client/bookmarkUtil.js

+  if (nextOrderSplit.length === 1) {
+    // Next order is an empty string
+    if (prevOrderSplit.length > 2) {
+      for (var i = 0; i < prevOrderSplit.length - 1; i++) {


it would be cleaner to replace this for loop with prevOrderSplit.forEach

actually it's even better to just do order = prevOrderSplit.join('.') + '.'

or order = prevOrder + '.' ?

the loop doesn't include the last element, it's not really forEach and not a join of all elements

ah sorry i missed that it doesn't include the last element.

in that case you could replace all these forloops with a regex like order = prevOrder.replace(/\.\d*$/, '') + '.'

diracdeltas · 2018-05-15T06:13:36Z

client/bookmarkUtil.js

+    }
+  } else if (prevOrderSplit.length === 1) {
+    // Prev order is an empty string
+    if (nextOrderSplit.length > 2) {


can replace this for loop with order = nextOrder + '.'

the same as above

diracdeltas · 2018-05-15T06:14:04Z

client/bookmarkUtil.js

+    }
+  } else {
+    if (prevOrderSplit.length > 2 && nextOrderSplit.length > 2) {
+      for (i = 0; i < prevOrderSplit.length - 1; i++) {


same as https://github.com/brave/sync/pull/204/files?utf8=%E2%9C%93&diff=unified#r188174101

the same as above

diracdeltas · 2018-05-15T06:18:01Z

client/bookmarkUtil.js

+        }
+      } else if (prevOrderSplit.length < nextOrderSplit.length) {
+        // Next order is longer than previous order
+        let lastNumberDiff = true


you can replace this and the following for loop with const lastNumberDiff = !nextOrder.startsWith(prevOrder) i think

what is nextOrder and prevOrder in your example? again the loop doesn't include the last element

the input parameters: https://github.com/brave/sync/pull/204/files#diff-bfda8eb33ecdb505fea0ed734376e047R49

though since it doesn't include the last element, this may not actually be simpler


        Merge branch 'staging' into bookmarks_ordering


        Remove extra space

SergeyZhukovsky added 3 commits May 11, 2018

Add order field and commands

f0faae1

Handle order commands

Loading status checks…

085c405

Travis fixes

Loading status checks…

0100b3f

SergeyZhukovsky requested a review from diracdeltas May 11, 2018

diracdeltas reviewed May 11, 2018

View changes

diracdeltas requested changes May 11, 2018

View changes

diracdeltas reviewed May 11, 2018

View changes

SergeyZhukovsky and others added 5 commits May 14, 2018

Unit tests, small fixes

Loading status checks…

2dce649

Travis fixes

Loading status checks…

32255b1

Add more tests and handle orders ending with 0

54b2057

Merge remote-tracking branch 'origin/staging' into bookmarks_ordering

Loading status checks…

ba75aef

Bookmarks order fix

Loading status checks…

3ea351e

diracdeltas reviewed May 15, 2018

View changes

SergeyZhukovsky added 2 commits May 15, 2018

Merge branch 'staging' into bookmarks_ordering

2a47764

Remove extra space

Loading status checks…

bd15a0f

diracdeltas approved these changes May 15, 2018

View changes

SergeyZhukovsky merged commit 75c1bb1 into staging May 15, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

SergeyZhukovsky mentioned this pull request May 15, 2018

Implement bookmarks ordering support #205

Closed

brave / sync

Bookmarks ordering #204

Bookmarks ordering #204

SergeyZhukovsky commented May 11, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

diracdeltas left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

		@@ -0,0 +1,4936 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

brave / sync

Join GitHub today

Bookmarks ordering #204

Bookmarks ordering #204

Conversation

SergeyZhukovsky commented May 11, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

diracdeltas left a comment

This comment has been minimized.

This comment has been minimized.

diracdeltas May 11, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

diracdeltas May 15, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

diracdeltas May 11, 2018 •

edited

diracdeltas May 15, 2018 •

edited