#1017 #864 Users should be able to rename and move files #41

leraradinovich · 2019-06-17T11:36:17Z

squallstar · 2019-06-17T12:58:10Z

interface.html

+
+<template id="template-move-modal-folders">
+  \{{#if (gt children.length 0)}}
+  <li


please use indentation with spaces as follows:

<li foo bar baz>

squallstar · 2019-06-17T12:59:51Z

interface.html

+          <ul class="move-modal-list">
+          </ul>
+          <div class="move-modal-empty-state">
+            <span>&#x1F633</span>


HTML Entity is missing semicolon:

😳

squallstar · 2019-06-17T13:00:07Z

js/interface.js

@@ -21,12 +25,15 @@ var currentAppId;
 var currentFolders;
 var currentFiles;
 var counterOrganization;
+var foldersForMoving = {};


var foldersForMoving = { navStack: [] };

squallstar · 2019-06-17T13:01:14Z

js/interface.js

+
+// Users selected folders tree (for breadcrumbs)
+function addPathToStack(id, name, orgId, appId, parentId, type) {
+  foldersForMoving['navStack'].push(


indentation needs to be fixed:

array.push({ });

squallstar · 2019-06-17T13:02:12Z

js/interface.js

+function addPathToStack(id, name, orgId, appId, parentId, type) {
+  foldersForMoving['navStack'].push(
+    {
+      id: Number(id)?Number(id):null,


Have you read the code guidelines? It should be a ? b : c (note the spaces)

Note: you can also change this operation to Number(id) || null

squallstar · 2019-06-17T13:08:07Z

js/interface.js

-    Fliplet.Media.Folders.create(options).then(addFolder);
-
-    $('.new-btn').click();
+    console.log('lastFolderSelected', lastFolderSelected)


console.log to be removed

squallstar · 2019-06-17T13:08:28Z

js/interface.js

-    $('.new-btn').click();
+    console.log('lastFolderSelected', lastFolderSelected)
+    Fliplet.Media.Folders.create(options).then( function (folder) {
+      if (!isCreatingInModal) {


looks like these two IFs should be combined into a single block

squallstar · 2019-06-17T13:08:52Z

js/interface.js

+    $('.move-modal-empty-state').removeClass('active');
+    $('[data-move-button]').attr('disabled');
+    foldersForMoving['navStack'] = [];
+    var type = $(this).find(":selected").attr('data-type');


please stick to single quotes whenever possible

squallstar · 2019-06-17T13:09:25Z

js/interface.js

+        $(this).attr('data-org-id'),
+        $(this).attr('data-app-id'),
+        $(this).attr('data-parent-id'),
+        'folder');


closing bracket should be on new line without indentation:

foo( 1, 2 );

squallstar · 2019-06-17T13:09:50Z

js/interface.js

+      folderId = null;
+    }
+
+    appId = appId ? appId : null;


use ternary operator: appId = appId || null;

squallstar · 2019-06-17T13:13:13Z

js/interface.js

+function excludeFolders(items) {
+  var parentItemIdOfSelectedItems = navStack[navStack.length-1].id;
+  var selectedIdPlaceInModal = foldersForMoving.navStack[foldersForMoving.navStack.length-1].id;
+  var searchResult = items;


Result is singular, but you're assigning a variable called items (plural). Which is correct?

fixed to plural name

squallstar · 2019-06-17T13:13:27Z

js/interface.js

+  }
+
+  return searchResult;
+


why this empty space?

squallstar · 2019-06-17T13:13:54Z

js/interface.js

+}
+
+// Create list of folders in modal
+function addFoldersToMoveModal(res) {


what's "res"? can you rename it to something more meaningful?

tonytlwu · 2019-06-17T13:54:51Z

js/interface.js

@@ -653,7 +890,7 @@ $('.file-manager-wrapper')
    if (!folderName) {
      return;
    }
-
+    


Empty lines should be completely empty, without spaces

squallstar · 2019-06-19T08:48:34Z

interface.html

+      data-org-id="\{{organizationId}}"
+      data-app-id="\{{appId}}"
+      data-parent-id="\{{parentId}}"
+  >


> should be on the previous line as per my previous comment

squallstar · 2019-06-19T08:49:12Z

js/interface.js

+      for (var i = 0; i < selectedIdForMoving.length; i++) {
+        return item.id !== selectedIdForMoving[i];
+      }
+    })


missing semicolon

squallstar · 2019-06-19T08:50:53Z

js/interface.js

+        )
+      }
+
+      updateMethod.then(function() {


please add a .catch

…emicolon, 2) added catch for search method, 3) fixed html structure

…and-move-files # Conflicts: # css/interface.css # js/interface.js

squallstar · 2019-07-17T09:10:19Z

@leraradinovich PR has conflicts and not all of my review notes were fixed.

#1017 #864 Users should be able to rename and move files

0803841

squallstar suggested changes Jun 17, 2019

View reviewed changes

squallstar reviewed Jun 17, 2019

View reviewed changes

tonytlwu reviewed Jun 17, 2019

View reviewed changes

leraradinovich added 2 commits June 17, 2019 20:16

#1017 #864 Users should be able to rename and move files - fixed commits

97fefba

#1017 #864 Users should be able to rename and move files - fixed commits

039a6b4

squallstar suggested changes Jun 19, 2019

View reviewed changes

leraradinovich added 2 commits June 20, 2019 14:29

#1017 #864 Users should be able to rename and move files - 1) added s…

4e63612

…emicolon, 2) added catch for search method, 3) fixed html structure

Merge branch 'master' into #1017-#864-Users-should-be-able-to-rename-…

6281e6b

…and-move-files # Conflicts: # css/interface.css # js/interface.js

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1017 #864 Users should be able to rename and move files #41

#1017 #864 Users should be able to rename and move files #41

leraradinovich commented Jun 17, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 17, 2019

leraradinovich Jun 19, 2019

tonytlwu Jun 17, 2019

leraradinovich Jun 19, 2019

squallstar Jun 19, 2019

squallstar Jun 19, 2019

squallstar Jun 19, 2019

squallstar commented Jul 17, 2019

#1017 #864 Users should be able to rename and move files #41

Are you sure you want to change the base?

#1017 #864 Users should be able to rename and move files #41

Conversation

leraradinovich commented Jun 17, 2019

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

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

squallstar commented Jul 17, 2019