Address feedback

app/assets/javascripts/groups/components/group_folder.vue

@@ -8,6 +8,7 @@ export default {
     baseGroup: {
       type: Object,
       required: false,
+      default: () => ({}),
     },
   },
 };
@@ -15,6 +16,12 @@ export default {
 <template>
   <ul class="content-list group-list-tree">
-    <group-item v-for="(group, index) in groups" :key="index" :group="group" :base-group="baseGroup" :collection="groups" />
+    <group-item
+      v-for="(group, index) in groups"
+      :key="index"
+      :group="group"
+      :base-group="baseGroup"
+      :collection="groups"
+    />
   </ul>
 </template>

app/assets/javascripts/groups/components/group_item.vue

@@ -102,71 +102,102 @@ export default {
     :id="groupDomId"
     :class="rowClass"
     >
-    <div class="group-row-contents">
-      <div class="controls">
+    <div
+      class="group-row-contents">
+      <div
+        class="controls">
         <a
           v-if="group.canEdit"
           class="edit-group btn"
           :href="group.editPath">
           <i aria-hidden="true" class="fa fa-cogs"></i>
         </a>
-        <a @click="onLeaveGroup"
+        <a
+          @click="onLeaveGroup"
           :href="group.leavePath"
           class="leave-group btn"
           title="Leave this group">
-          <i aria-hidden="true" class="fa fa-sign-out"></i>
+          <i
+            aria-hidden="true"
+            class="fa fa-sign-out">
+          </i>
         </a>
       </div>
-
-      <div class="stats">
-        <span class="number-projects">
-          <i aria-hidden="true" class="fa fa-bookmark"></i>
+      <div
+        class="stats">
+        <span
+          class="number-projects">
+          <i
+            aria-hidden="true"
+            class="fa fa-bookmark">
+          </i>
           {{group.numberProjects}}
         </span>
-        <span class="number-users">
-          <i aria-hidden="true" class="fa fa-users"></i>
+        <span
+          class="number-users">
+          <i
+            aria-hidden="true"
+            class="fa fa-users">
+          </i>
           {{group.numberUsers}}
         </span>
-        <span class="group-visibility">
-          <i aria-hidden="true" :class="visibilityIcon"></i>
+        <span
+          class="group-visibility">
+          <i
+            aria-hidden="true"
+            :class="visibilityIcon">
+          </i>
         </span>
       </div>
-
-      <div class="folder-toggle-wrap">
-        <span class="folder-caret" v-if="group.hasSubgroups">
+      <div
+        class="folder-toggle-wrap">
+        <span
+          class="folder-caret"
+          v-if="group.hasSubgroups">
           <i
             v-if="group.isOpen"
-            class="fa fa-caret-down" />
+            class="fa fa-caret-down">
+          </i>
           <i
             v-if="!group.isOpen"
-            class="fa fa-caret-right" />
+            class="fa fa-caret-right">
+          </i>
         </span>
-
         <span class="folder-icon">
           <i
             v-if="group.isOpen"
             class="fa fa-folder-open"
-            aria-hidden="true"></i>
+            aria-hidden="true">
+          </i>
           <i
             v-if="!group.isOpen"
-            class="fa fa-folder"></i>
+            class="fa fa-folder"
+            aria-hidden="true">
+          </i>
         </span>
       </div>
-
-      <div class="avatar-container s40 hidden-xs">
-        <a :href="group.webUrl">
-          <img class="avatar s40" :src="group.avatarUrl" />
+      <div
+        class="avatar-container s40 hidden-xs">
+        <a
+          :href="group.webUrl">
+          <img
+            class="avatar s40"
+            :src="group.avatarUrl"
+          />
         </a>
       </div>
-
-      <div class="title">
-        <a :href="group.webUrl">{{fullPath}}</a>
-      </div>
-
-      <div class="description">
-        {{group.description}}
+      <div
+        class="title">
+        <a
+          :href="group.webUrl">{{fullPath}}</a>
       </div>
+      <div
+        class="description">{{group.description}}</div>
     </div>
-    <group-folder v-if="group.isOpen && hasGroups" :groups="group.subGroups" :baseGroup="group" />
+    <group-folder
+      v-if="group.isOpen && hasGroups"
+      :groups="group.subGroups"
+      :baseGroup="group"
+    />
   </li>
 </template>

app/assets/javascripts/groups/components/groups.vue

 <script>
-import TablePaginationComponent from '~/vue_shared/components/table_pagination.vue';
+import TablePagination from '~/vue_shared/components/table_pagination.vue';
 import eventHub from '../event_hub';
 export default {
-  components: {
-    'gl-pagination': TablePaginationComponent,
-  },
   props: {
     groups: {
       type: Object,
@@ -16,6 +13,9 @@ export default {
       required: true,
     },
   },
+  components: {
+    TablePagination,
+  },
   methods: {
     change(page) {
       const filterGroupsParam = gl.utils.getParameterByName('filter_groups');
@@ -28,9 +28,12 @@ export default {
 <template>
   <div class="groups-list-tree-container">
-    <group-folder :groups="groups" />
-    <gl-pagination
+    <group-folder
+      :groups="groups"
+    />
+    <table-pagination
       :change="change"
-      :pageInfo="pageInfo" />
+      :pageInfo="pageInfo"
+    />
   </div>
 </template>

app/assets/javascripts/groups/index.js

@@ -10,7 +10,7 @@ import GroupsService from './services/groups_service';
 import eventHub from './event_hub';
 document.addEventListener('DOMContentLoaded', () => {
-  const el = document.querySelector('#dashboard-group-app');
+  const el = document.getElementById('dashboard-group-app');
   // Don't do anything if element doesn't exist (No groups)
   // This is for when the user enters directly to the page via URL
@@ -74,27 +74,28 @@ document.addEventListener('DOMContentLoaded', () => {
         }
         getGroups = this.service.getGroups(parentId, page, filterGroups, sort);
-        getGroups.then((response) => {
-          this.updateGroups(response.json(), parentGroup);
-        })
-        .finally(() => {
-          this.isLoading = false;
-        })
-        .catch(this.handleErrorResponse);
+        getGroups
+          .then(response => response.json())
+          .then((response) => {
+            this.isLoading = false;
+
+            this.updateGroups(response, parentGroup);
+          })
+          .catch(this.handleErrorResponse);
         return getGroups;
       },
       fetchPage(page, filterGroups, sort) {
         this.isLoading = true;
-        this.service.getGroups(null, page, filterGroups, sort)
+        return this.service
+          .getGroups(null, page, filterGroups, sort)
           .then((response) => {
+            this.isLoading = false;
+            $.scrollTo(0);
+
             this.updateGroups(response.json());
             this.updatePagination(response.headers);
-            $.scrollTo(0);
-          })
-          .finally(() => {
-            this.isLoading = false;
           })
           .catch(this.handleErrorResponse);
       },
@@ -104,19 +105,18 @@ document.addEventListener('DOMContentLoaded', () => {
           this.fetchGroups(parentGroup);
         }
-        GroupsStore.toggleSubGroups(parentGroup);
+        this.store.toggleSubGroups(parentGroup);
       },
       leaveGroup(group, collection) {
         this.service.leaveGroup(group.leavePath)
           .then((response) => {
+            $.scrollTo(0);
+
             this.store.removeGroup(group, collection);
             // eslint-disable-next-line no-new
             new Flash(response.json().notice, 'notice');
           })
-          .finally(() => {
-            $.scrollTo(0);
-          })
           .catch((response) => {
             let message = 'An error occurred. Please try again.';
@@ -135,10 +135,20 @@ document.addEventListener('DOMContentLoaded', () => {
         this.store.storePagination(headers);
       },
       handleErrorResponse() {
+        this.isLoading = false;
+        $.scrollTo(0);
+
         // eslint-disable-next-line no-new
         new Flash('An error occurred. Please try again.');
       },
     },
+    created() {
+      eventHub.$on('fetchPage', this.fetchPage);
+      eventHub.$on('toggleSubGroups', this.toggleSubGroups);
+      eventHub.$on('leaveGroup', this.leaveGroup);
+      eventHub.$on('updateGroups', this.updateGroups);
+      eventHub.$on('updatePagination', this.updatePagination);
+    },
     beforeMount() {
       let groupFilterList = null;
       const form = document.querySelector('form#group-filter-form');
@@ -155,19 +165,11 @@ document.addEventListener('DOMContentLoaded', () => {
       groupFilterList = new GroupFilterableList(opts);
       groupFilterList.initSearch();
-
-      eventHub.$on('fetchPage', this.fetchPage);
-      eventHub.$on('toggleSubGroups', this.toggleSubGroups);
-      eventHub.$on('leaveGroup', this.leaveGroup);
-      eventHub.$on('updateGroups', this.updateGroups);
-      eventHub.$on('updatePagination', this.updatePagination);
     },
     mounted() {
       this.fetchGroups()
         .then((response) => {
           this.updatePagination(response.headers);
-        })
-        .finally(() => {
           this.isLoading = false;
         })
         .catch(this.handleErrorResponse);

app/assets/javascripts/groups/stores/groups_store.js

@@ -140,7 +140,7 @@ export default class GroupsStore {
   }
   // eslint-disable-next-line class-methods-use-this
-  static toggleSubGroups(toggleGroup) {
+  toggleSubGroups(toggleGroup) {
     const group = toggleGroup;
     group.isOpen = !group.isOpen;
     return group;

app/assets/stylesheets/framework/lists.scss

@@ -340,21 +340,21 @@ ul.indent-list {
       }
     }
   }
-  
+
   .group-row {
     padding: 0;
     border: none;
   }
   .group-row-contents {
-    padding: 10px 10px 8px 10px;
+    padding: 10px 10px 8px;
     border-top: solid 1px transparent;
     border-bottom: solid 1px $white-normal;
-    &:hover{
+    &:hover {
       border-color: $row-hover-border;
       background-color: $row-hover;
-      cursor: pointer; 
+      cursor: pointer;
     }
   }
 }

spec/javascripts/groups/group_item_spec.js

@@ -3,39 +3,64 @@ import groupItemComponent from '~/groups/components/group_item.vue';
 import GroupsStore from '~/groups/stores/groups_store';
 import { group1 } from './mock_data';
-fdescribe('Groups Component', () => {
+describe('Groups Component', () => {
   let GroupItemComponent;
   let component;
   let store;
   let group;
-  beforeEach((done) => {
-    GroupItemComponent = Vue.extend(groupItemComponent);
-    store = new GroupsStore();
-    group = store.decorateGroup(group1);
-    component = new GroupItemComponent({
-      propsData: {
-        group,
-      },
-    }).$mount();
-    Vue.nextTick(() => {
-      done();
+  describe('group with default data', () => {
+    beforeEach((done) => {
+      GroupItemComponent = Vue.extend(groupItemComponent);
+      store = new GroupsStore();
+      group = store.decorateGroup(group1);
+      component = new GroupItemComponent({
+        propsData: {
+          group,
+        },
+      }).$mount();
+      Vue.nextTick(() => {
+        done();
+      });
     });
-  });
-  it('should render the group item', () => {
-    expect(component.$el.classList.contains('group-row')).toBe(true);
-    expect(component.$el.querySelector('.number-projects').textContent).toContain(group.numberProjects);
-    expect(component.$el.querySelector('.number-users').textContent).toContain(group.numberUsers);
-    expect(component.$el.querySelector('.group-visibility')).toBeDefined();
-    expect(component.$el.querySelector('.avatar-container')).toBeDefined();
-    expect(component.$el.querySelector('.title').textContent).toContain(group.name);
-    expect(component.$el.querySelector('.description').textContent).toContain(group.description);
-    expect(component.$el.querySelector('.edit-group')).toBeDefined();
-    expect(component.$el.querySelector('.leave-group')).toBeDefined();
+    it('should render the group item correctly', () => {
+      expect(component.$el.classList.contains('group-row')).toBe(true);
+      expect(component.$el.classList.contains('.no-description')).toBe(false);
+      expect(component.$el.querySelector('.number-projects').textContent).toContain(group.numberProjects);
+      expect(component.$el.querySelector('.number-users').textContent).toContain(group.numberUsers);
+      expect(component.$el.querySelector('.group-visibility')).toBeDefined();
+      expect(component.$el.querySelector('.avatar-container')).toBeDefined();
+      expect(component.$el.querySelector('.title').textContent).toContain(group.name);
+      expect(component.$el.querySelector('.description').textContent).toContain(group.description);
+      expect(component.$el.querySelector('.edit-group')).toBeDefined();
+      expect(component.$el.querySelector('.leave-group')).toBeDefined();
+    });
   });
-  // TODO: check for no description class when group has no description
+  describe('group without description', () => {
+    beforeEach((done) => {
+      GroupItemComponent = Vue.extend(groupItemComponent);
+      store = new GroupsStore();
+      group1.description = '';
+      group = store.decorateGroup(group1);
+
+      component = new GroupItemComponent({
+        propsData: {
+          group,
+        },
+      }).$mount();
+
+      Vue.nextTick(() => {
+        done();
+      });
+    });
+
+    it('should render group item correctly', () => {
+      expect(component.$el.querySelector('.description').textContent).toBe('');
+      expect(component.$el.classList.contains('.no-description')).toBe(false);
+    });
+  });
 });

spec/javascripts/groups/groups_spec.js

@@ -5,7 +5,7 @@ import groupsComponent from '~/groups/components/groups.vue';
 import GroupsStore from '~/groups/stores/groups_store';
 import { groupsData } from './mock_data';
-fdescribe('Groups Component', () => {
+describe('Groups Component', () => {
   let GroupsComponent;
   let store;
   let component;

spec/javascripts/groups/mock_data.js

@@ -2,7 +2,7 @@ const group1 = {
   id: '12',
   name: 'level1',
   path: 'level1',
-  description: '',
+  description: 'foo',
   visibility: 'public',
   avatar_url: null,
   web_url: 'http://localhost:3000/groups/level1',
@@ -24,7 +24,7 @@ const group14 = {
   id: 1128,
   name: 'level4',
   path: 'level4',
-  description: '',
+  description: 'foo',
   visibility: 'public',
   avatar_url: null,
   web_url: 'http://localhost:3000/groups/level1/level2/level3/level4',
@@ -45,7 +45,7 @@ const group2 = {
   id: 1119,
   name: 'devops',
   path: 'devops',
-  description: '',
+  description: 'foo',
   visibility: 'public',
   avatar_url: null,
   web_url: 'http://localhost:3000/groups/devops',
@@ -66,7 +66,7 @@ const group21 = {
   id: 1120,
   name: 'chef',
   path: 'chef',
-  description: '',
+  description: 'foo',
   visibility: 'public',
   avatar_url: null,
   web_url: 'http://localhost:3000/groups/devops/chef',