Merge pull request #4318 from edx/gprice/forum-sidebar-debt

Reduce tech debt in forum sidebar

common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee

@@ -147,7 +147,7 @@ describe "DiscussionThreadListView", ->
                 </div>
             </div>
         </script>
-        <div class="sidebar"></div>
+        <div class="forum-nav"></div>
         """
         @threads = [
           makeThreadWithProps({
@@ -176,7 +176,7 @@ describe "DiscussionThreadListView", ->
         spyOn($, "ajax")
 
         @discussion = new Discussion([])
-        @view = new DiscussionThreadListView({collection: @discussion, el: $(".sidebar")})
+        @view = new DiscussionThreadListView({collection: @discussion, el: $(".forum-nav")})
         @view.render()
 
     makeThreadWithProps = (props) ->
@@ -196,7 +196,7 @@ describe "DiscussionThreadListView", ->
 
     makeView = (discussion) ->
       return new DiscussionThreadListView(
-          el: $(".sidebar"),
+          el: $(".forum-nav"),
           collection: discussion
       )
 

common/static/coffee/src/discussion/discussion_router.coffee

@@ -8,7 +8,7 @@ if Backbone?
         @discussion = options['discussion']
         @course_settings = options['course_settings']
 
-        @nav = new DiscussionThreadListView(collection: @discussion, el: $(".sidebar"))
+        @nav = new DiscussionThreadListView(collection: @discussion, el: $(".forum-nav"))
         @nav.on "thread:selected", @navigateToThread
         @nav.on "thread:removed", @navigateToAllThreads
         @nav.on "threads:rendered", @setActiveThread

common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee

@@ -89,7 +89,7 @@ if Backbone?
       discussionsBodyTop = if discussionBody[0] then discussionBody.offset().top
       discussionsBodyBottom = discussionsBodyTop + discussionBody.outerHeight()
 
-      sidebar = $(".sidebar")
+      sidebar = $(".forum-nav")
       if scrollTop > discussionsBodyTop - @sidebar_padding
         sidebar.css('top', scrollTop - discussionsBodyTop + @sidebar_padding);
       else
@@ -107,8 +107,9 @@ if Backbone?
 
       headerHeight = @$(".forum-nav-header").outerHeight()
       refineBarHeight = @$(".forum-nav-refine-bar").outerHeight()
+      browseFilterHeight = @$(".forum-nav-browse-filter").outerHeight()
       @$('.forum-nav-thread-list').css('height', (sidebarHeight - headerHeight - refineBarHeight - 2) + 'px')
-      @$('.forum-nav-browse-menu-wrapper').css('height', (sidebarHeight - headerHeight - 2) + 'px')
+      @$('.forum-nav-browse-menu').css('height', (sidebarHeight - headerHeight - browseFilterHeight - 2) + 'px')
 
 
     # Because we want the behavior that when the body is clicked the menu is
@@ -267,6 +268,7 @@ if Backbone?
         @$(".forum-nav-thread-list-wrapper").hide()
         $(".forum-nav-browse-filter-input").focus()
         $("body").bind "click", @hideBrowseMenu
+        @updateSidebar()
 
     hideBrowseMenu: =>
       if @isBrowseMenuVisible()
@@ -274,6 +276,7 @@ if Backbone?
         @$(".forum-nav-browse-menu-wrapper").hide()
         @$(".forum-nav-thread-list-wrapper").show()
         $("body").unbind "click", @hideBrowseMenu
+        @updateSidebar()
 
     toggleBrowseMenu: (event) =>
       event.preventDefault()

common/test/acceptance/pages/lms/discussion.py

@@ -343,7 +343,7 @@ class DiscussionUserProfilePage(CoursePage):
 
 class DiscussionTabHomePage(CoursePage, DiscussionPageMixin):
 
-    ALERT_SELECTOR = ".discussion-body .sidebar .search-alert"
+    ALERT_SELECTOR = ".discussion-body .forum-nav .search-alert"
 
     def __init__(self, browser, course_id):
         super(DiscussionTabHomePage, self).__init__(browser, course_id)

lms/static/sass/discussion/_discussion.scss

@@ -436,7 +436,6 @@ body.discussion {
   }
 
   section.user-profile {
-    @extend .sidebar;
     display: table-cell;
     border-right: 1px solid #ddd;
     border-radius: 3px 0 0 3px;

lms/static/sass/discussion/elements/_navigation.scss

 .forum-nav {
   @include box-sizing(border-box);
   float: left;
+  position: relative;
+  width: 31%;
   border: 1px solid #aaa;
   border-radius: 3px;
 }
@@ -27,6 +29,7 @@
   }
 
   .icon {
+    @include font-size(14);
     margin-right: ($baseline/4);
   }
 }
@@ -64,7 +67,6 @@
 // Browse menu
 // -----------
 .forum-nav-browse-menu-wrapper {
-  overflow-y: scroll;
   border-bottom: 1px solid $gray-l3;
   background: $gray-l5;
 }
@@ -87,6 +89,27 @@
   width: 100%;
 }
 
+.forum-nav-browse-menu {
+  @include font-size(14);
+  overflow-y: scroll;
+  list-style: none;
+}
+
+.forum-nav-browse-submenu {
+  padding-left: $baseline;
+  list-style: none;
+}
+
+.forum-nav-browse-title {
+  display: block;
+  border-bottom: 1px solid $gray-l3;
+  padding: ($baseline/2) ($baseline/2);
+
+  &:hover, &:focus {
+    background: $forum-color-active-thread;
+  }
+}
+
 .forum-nav-browse-title .icon {
   margin-right: ($baseline/2);
 }
@@ -127,14 +150,25 @@
 // -----------
 .forum-nav-thread-list {
   overflow-y: scroll;
+  list-style: none;
 }
 
 .forum-nav-thread {
   border-bottom: 1px solid $gray-l3;
+  background-color: $gray-l5;
+
+  &.is-unread {
+    background-color: $white;
+  }
 }
 
 .forum-nav-thread-link {
-  @include clearfix();
+  display: block;
+  padding: ($baseline/4) ($baseline/2);
+
+  &.is-active, &:hover, &:focus {
+    background-color: $forum-color-active-thread;
+  }
 }
 
 %forum-nav-thread-wrapper {
@@ -162,8 +196,10 @@
   @extend %t-weight4;
   @include font-size(9);
   display: inline;
+  margin-top: ($baseline/4);
   border: 1px solid;
   border-radius: 3px;
+  padding: 1px 6px;
   text-transform: uppercase;
   white-space: nowrap;
 
@@ -265,13 +301,25 @@
   }
 }
 
+.forum-nav-load-more {
+  border-bottom: 1px solid $gray-l3;
+  background-color: $gray-l5;
+}
+
 %forum-nav-load-more-content {
+  display: block;
+  padding: $baseline 0;
   text-align: center;
 }
 
 .forum-nav-load-more-link {
   @extend %forum-nav-load-more-content;
   color: $link-color;
+
+  &:hover, &:focus {
+    color: $link-color;
+    background-color: $forum-color-active-thread;
+  }
 }
 
 .forum-nav-loading {

lms/static/sass/discussion/utilities/_developer.scss

@@ -19,7 +19,7 @@
 // --------------------
 body.discussion {
 
-  .sidebar {
+  .forum-nav {
 
     // wrapper for multiple alerts
     .search-alerts {

lms/static/sass/discussion/utilities/_shame.scss

@@ -29,15 +29,6 @@
   position: relative;
 }
 
-// The sidebar class does a lot of things that we don't want in the thread list;
-// the following rules contain styling that is necessary and would otherwise
-// reside in elements/_navigation.scss if the sidebar styling did not make the
-// !important directive necessary.
-
-.forum-nav {
-  width: 31% !important;
-}
-
 // ------------------------
 // navigation - browse menu
 // ------------------------
@@ -62,22 +53,13 @@
   font-size: 12px !important;
 }
 
-// The sidebar class does a lot of things that we don't want in the thread list;
-// the following rules contain styling that is necessary and would otherwise
-// reside in elements/_navigation.scss if the sidebar styling did not make the
-// !important directive necessary.
-
-.forum-nav-browse-title {
-  border-bottom: 1px solid $gray-l3 !important;
-  padding: ($baseline/2) ($baseline/2) !important;
-
-  &:hover, &:focus {
-    background: $forum-color-active-thread !important;
-  }
+// Override global ul rules
+.forum-nav-browse-menu, .forum-nav-browse-submenu {
+  margin: 0;
 }
 
-.forum-nav-browse-submenu {
-  padding-left: $baseline !important;
+.forum-nav-browse-menu {
+  padding-left: 0;
 }
 
 // --------------------------------
@@ -93,51 +75,15 @@
 // navigation - thread list
 // --------------------------------
 
-// The sidebar class does a lot of things that we don't want in the thread list;
-// the following rules contain styling that is necessary and would otherwise
-// reside in elements/_navigation.scss if the sidebar styling did not make the
-// !important directive necessary.
-
-.forum-nav-thread {
-  background-color: $gray-l5 !important;
-
-  &.is-unread {
-    background-color: $white !important;
-  }
-}
-
-.forum-nav-thread-link {
-  padding: ($baseline/4) ($baseline/2) !important;
-
-  &.is-active, &:hover, &:focus {
-    background-color: $forum-color-active-thread !important;
-  }
-}
-
-li[class*=forum-nav-thread-label-] {
-  margin-top: ($baseline/4) !important;
-  padding: 1px 6px !important;
-}
-
-.forum-nav-load-more {
-  border-bottom: 1px solid $gray-l3 !important;
-  background-color: $gray-l5 !important;
-}
-
-.forum-nav-load-more-link {
-  &:hover, &:focus {
-    color: $link-color !important;
-    background-color: $forum-color-active-thread !important;
-  }
-}
-
-.forum-nav-load-more-link, .forum-nav-loading {
-  padding: $baseline 0 !important;
-}
-
 // The following rules would be unnecessary but for broadly scoped rules defined
 // elsewhere in our CSS.
 
+// Override global ul rules
+.forum-nav-thread-list, .forum-nav-thread-labels {
+  margin: 0;
+  padding-left: 0;
+}
+
 li[class*=forum-nav-thread-label-] {
   // Override global span rules
   span {

lms/templates/discussion/index.html

@@ -37,7 +37,7 @@
          data-user-cohort-id="${user_cohort}"
          data-course-settings="${course_settings}">
     <div class="discussion-body">
-        <div class="sidebar forum-nav"></div>
+        <div class="forum-nav"></div>
         <div class="discussion-column">
         </div>
     </div>