LMS: Student Notes - View All/Search Final Styling (UX-1331)

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

@@ -142,13 +142,13 @@ def is_closable(self):
         """
         Indicates if tab is closable or not.
         """
-        return self.q(css="{} .btn-close".format(self.TAB_SELECTOR)).present
+        return self.q(css="{} .action-close".format(self.TAB_SELECTOR)).present
 
     def close(self):
         """
         Closes the tab.
         """
-        self.q(css="{} .btn-close".format(self.TAB_SELECTOR)).first.click()
+        self.q(css="{} .action-close".format(self.TAB_SELECTOR)).first.click()
 
     @property
     def children(self):

lms/djangoapps/edxnotes/tests.py

@@ -803,7 +803,7 @@ def test_edxnotes_view_is_enabled(self, mock_get_notes):
         """
         enable_edxnotes_for_the_course(self.course, self.user.id)
         response = self.client.get(self.notes_page_url)
-        self.assertContains(response, '<h1 class="page-title">Notes')
+        self.assertContains(response, 'Highlights and notes you\'ve made in course content')
 
     @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False})
     def test_edxnotes_view_is_disabled(self):

lms/static/coffee/src/calculator.coffee

@@ -72,7 +72,7 @@ class @Calculator
       .attr
         'title': text
         'aria-expanded': isExpanded
-      .text text
+      .find('.utility-control-label').text text
 
     $calc.toggleClass 'closed'
 

lms/static/js/edxnotes/views/tab_item.js

@@ -10,7 +10,7 @@ function (gettext, _, Backbone, templateUtils) {
         events: {
             'click': 'selectHandler',
             'click a': function (event) { event.preventDefault(); },
-            'click .btn-close': 'closeHandler'
+            'click .action-close': 'closeHandler'
         },
 
         initialize: function (options) {

lms/static/js/edxnotes/views/tab_view.js

@@ -48,7 +48,7 @@ define([
          * Renders content for the view.
          */
         render: function () {
-            this.showLoadingIndicator();
+            this.hideErrorMessage().showLoadingIndicator();
             // If the view is already rendered, destroy it.
             this.destroySubView();
             this.renderContent().always(this.hideLoadingIndicator);
@@ -98,33 +98,40 @@ define([
          */
         showLoadingIndicator: function() {
             this.getLoadingIndicator().removeClass('is-hidden');
+            return this;
         },
 
         /**
          * Hides the page's loading indicator.
          */
         hideLoadingIndicator: function() {
             this.getLoadingIndicator().addClass('is-hidden');
+            return this;
         },
 
 
         /**
          * Shows error message.
          */
         showErrorMessage: function (message) {
-            this.$('.inline-error')
-                .text(message)
+            this.$('.wrapper-msg')
                 .removeClass('is-hidden')
+                .find('.msg-content .copy').text(message)
+                .closest('.msg')
                 .focus();
+
+            return this;
         },
 
         /**
          * Hides error message.
          */
         hideErrorMessage: function () {
-            this.$('.inline-error')
-                .text('')
-                .addClass('is-hidden');
+            this.$('.wrapper-msg')
+                .addClass('is-hidden')
+                .find('.msg-content .copy').text('');
+
+            return this;
         }
     });
 

lms/static/js/edxnotes/views/tabs/search_results.js

@@ -39,7 +39,7 @@ define([
                 ].join(' ')
             },
             renderContent: function () {
-                var message = gettext('No results found for "%(query_string)s".');
+                var message = gettext('No results found for "%(query_string)s". Please try searching again.');
 
                 this.$el.append($('<p></p>', {
                     text: interpolate(message, {

lms/static/js/edxnotes/views/toggle_notes_factory.js

@@ -1,8 +1,9 @@
 ;(function (define, undefined) {
 'use strict';
 define([
-    'jquery', 'underscore', 'backbone', 'gettext', 'js/edxnotes/views/visibility_decorator'
-], function($, _, Backbone, gettext, EdxnotesVisibilityDecorator) {
+    'jquery', 'underscore', 'backbone', 'gettext',
+    'annotator', 'js/edxnotes/views/visibility_decorator'
+], function($, _, Backbone, gettext, Annotator, EdxnotesVisibilityDecorator) {
     var ToggleNotesView = Backbone.View.extend({
         events: {
             'click .action-toggle-notes': 'toogleHandler'
@@ -14,7 +15,9 @@ define([
             this.visibility = options.visibility;
             this.visibilityUrl = options.visibilityUrl;
             this.checkboxIcon = this.$('.checkbox-icon');
-            this.$('.action-toggle-notes').removeClass('is-disabled');
+            this.actionLink = this.$('.action-toggle-notes');
+            this.actionLink.removeClass('is-disabled');
+            this.notification = new Annotator.Notification();
         },
 
         toogleHandler: function (event) {
@@ -28,18 +31,20 @@ define([
             if (this.visibility) {
                 _.each($('.edx-notes-wrapper'), EdxnotesVisibilityDecorator.enableNote);
                 this.checkboxIcon.removeClass('icon-check-empty').addClass('icon-check');
+                this.actionLink.addClass('is-active');
             } else {
                 EdxnotesVisibilityDecorator.disableNotes();
                 this.checkboxIcon.removeClass('icon-check').addClass('icon-check-empty');
+                this.actionLink.removeClass('is-active');
             }
         },
 
         hideErrorMessage: function() {
-            this.$('.edx-notes-visibility-error').text('');
+            this.notification.hide();
         },
 
         showErrorMessage: function(message) {
-            this.$('.edx-notes-visibility-error').text(message);
+            this.notification.show(message, Annotator.Notification.ERROR);
         },
 
         sendRequest: function () {

lms/static/js/fixtures/edxnotes/edxnotes.html

@@ -15,11 +15,18 @@ <h1 class="page-title">
         </div>
     </div>
 
+    <div class="wrapper-msg is-hidden error urgency-high inline-error">
+        <div class="msg msg-error" tabindex="-1">
+          <div class="msg-content">
+            <p class="copy"></p>
+          </div>
+        </div>
+    </div>
+
     <section class="wrapper-tabs">
         <div class="tab-list is-hidden">
             <h2 id="tab-view" class="tabs-label">View notes by:</h2>
         </div>
-        <div class="inline-error is-hidden" tabindex="-1"></div>
         <div class="ui-loading" tabindex="-1">
             <p><span class="spin"><i class="icon-refresh"></i></span> <span class="copy">Loading...</span></p>
         </div>

lms/static/js/fixtures/edxnotes/toggle_notes.html

@@ -1,7 +1,6 @@
 <div class="action-inline edx-notes-visibility">
-    <a href="#" class="action-toggle-notes is-disabled" role="button" aria-pressed="">
+    <a href="#" class="action-toggle-notes is-disabled is-active" role="button" aria-pressed="">
         <i class="checkbox-icon icon-check"></i>
         Show notes
     </a>
-    <div class="edx-notes-visibility-error"></div>
 </div>

lms/static/js/spec/edxnotes/views/note_item_spec.js

@@ -29,7 +29,7 @@ define([
             view.$('.note-excerpt-more-link').click();
 
             expect(view.$el).toContainText(Helpers.LONG_TEXT);
-            expect(view.$el).toContainText('(Show less)');
+            expect(view.$el).toContainText('Less');
 
             view = getView({quote: Helpers.SHORT_TEXT});
             expect(view.$el).not.toContain('.note-excerpt-more-link');

lms/static/js/spec/edxnotes/views/tab_item_spec.js

@@ -47,7 +47,7 @@ define([
             var secondItem = this.tabsList.$('#second-item');
 
             expect(this.tabsList.$('.tab')).toHaveLength(2);
-            secondItem.find('.btn-close').click();
+            secondItem.find('.action-close').click();
             expect(this.tabsList.$('.tab')).toHaveLength(1);
         });
     });

lms/static/js/spec/edxnotes/views/tab_view_spec.js

@@ -78,7 +78,7 @@ define([
         it('can remove the content if tab is closed', function () {
             var view = getView(this.tabsCollection);
             view.onClose =  jasmine.createSpy();
-            view.$('.tab .btn-close').click();
+            view.$('.tab .action-close').click();
             expect(view.$('.tab')).toHaveLength(0);
             expect(view.$('.wrapper-tabs')).not.toContainHtml('<p>test view content</p>');
             expect(view.tabModel).toBeNull();
@@ -95,15 +95,24 @@ define([
 
         it('can show/hide error messages', function () {
             var view = getView(this.tabsCollection),
-                errorHolder = view.$('.inline-error');
+                errorHolder = view.$('.wrapper-msg');
             view.showErrorMessage('<p>error message is here</p>');
             expect(errorHolder).not.toHaveClass('is-hidden');
-            expect(errorHolder).toBeFocused();
-            expect(errorHolder).toContainText('<p>error message is here</p>');
+            expect(errorHolder.find('.msg')).toBeFocused();
+            expect(errorHolder.find('.copy')).toContainText('<p>error message is here</p>');
 
             view.hideErrorMessage();
             expect(errorHolder).toHaveClass('is-hidden');
-            expect(errorHolder).toBeEmpty();
+            expect(errorHolder.find('.copy')).toBeEmpty();
+        });
+
+        it('should hide error messages before rendering', function () {
+            var view = getView(this.tabsCollection),
+                errorHolder = view.$('.wrapper-msg');
+            view.showErrorMessage('<p>error message is here</p>');
+            view.render();
+            expect(errorHolder).toHaveClass('is-hidden');
+            expect(errorHolder.find('.copy')).toBeEmpty();
         });
     });
 });

lms/static/js/spec/edxnotes/views/tabs/search_results_spec.js

@@ -167,16 +167,16 @@ define([
                 })
             );
 
-            expect(view.$('.inline-error')).not.toHaveClass('is-hidden');
-            expect(view.$('.inline-error')).toContainText('test error message');
+            expect(view.$('.wrapper-msg')).not.toHaveClass('is-hidden');
+            expect(view.$('.wrapper-msg .copy')).toContainText('test error message');
             expect(view.$('.note-highlight')).not.toExist();
             expect(view.$('.ui-loading')).toHaveClass('is-hidden');
 
             submitForm(view.searchBox, 'Second');
             AjaxHelpers.respondWithJson(requests, responseJson);
 
-            expect(view.$('.inline-error')).toHaveClass('is-hidden');
-            expect(view.$('.inline-error')).toBeEmpty();
+            expect(view.$('.wrapper-msg')).toHaveClass('is-hidden');
+            expect(view.$('.wrapper-msg .copy')).toBeEmpty();
             expect(view.$('.note-highlight')).toExist();
         });
 

lms/static/js/spec/edxnotes/views/toggle_notes_factory_spec.js

@@ -37,6 +37,7 @@ define([
         afterEach(function () {
             VisibilityDecorator._setVisibility(null);
             _.invoke(Annotator._instances, 'destroy');
+            $('.annotator-notice').remove();
         });
 
         it('can toggle notes', function() {
@@ -45,10 +46,12 @@ define([
             expect(this.button).not.toHaveClass('is-disabled');
             expect(this.icon).toHaveClass('icon-check');
             expect(this.icon).not.toHaveClass('icon-check-empty');
+            expect(this.button).toHaveClass('is-active');
 
             this.button.click();
             expect(this.icon).toHaveClass('icon-check-empty');
             expect(this.icon).not.toHaveClass('icon-check');
+            expect(this.button).not.toHaveClass('is-active');
             expect(Annotator._instances).toHaveLength(0);
 
             AjaxHelpers.expectJsonRequest(requests, 'PUT', '/test_url', {
@@ -59,6 +62,7 @@ define([
             this.button.click();
             expect(this.icon).toHaveClass('icon-check');
             expect(this.icon).not.toHaveClass('icon-check-empty');
+            expect(this.button).toHaveClass('is-active');
             expect(Annotator._instances).toHaveLength(2);
 
             AjaxHelpers.expectJsonRequest(requests, 'PUT', '/test_url', {
@@ -69,17 +73,20 @@ define([
 
         it('can handle errors', function() {
             var requests = AjaxHelpers.requests(this),
-                errorContainer = $('.edx-notes-visibility-error');
+                errorContainer = $('.annotator-notice');
 
             this.button.click();
             AjaxHelpers.respondWithError(requests);
             expect(errorContainer).toContainText(
                 'Cannot save your state. This may be happening because of an error with our server or your internet connection. Try refreshing the page or making sure you are online.'
             );
+            expect(errorContainer).toBeVisible();
+            expect(errorContainer).toHaveClass('annotator-notice-show');
+            expect(errorContainer).toHaveClass('annotator-notice-error');
 
             this.button.click();
             AjaxHelpers.respondWithJson(requests, {});
-            expect(errorContainer).toBeEmpty();
+            expect(errorContainer).not.toHaveClass('annotator-notice-show');
         });
     });
 });

lms/static/sass/_developer.scss

@@ -117,9 +117,3 @@
     padding-left: ($baseline/4);
   }
 }
-
-.edx-notes-visibility {
-  .error {
-    color: $red;
-  }
-}

lms/static/sass/base/_mixins.scss

@@ -181,3 +181,25 @@
 %no-border-right {
   border-right: none;
 }
+
+// outline
+%no-outline {
+  outline: none;
+}
+
+// shame-based mixins to centrally override poor styling
+%shame-link-base {
+  color: $link-color;
+
+  &:hover, &:focus {
+    color: saturate($link-color, 50%);
+  }
+}
+
+%shame-link-text {
+  @extend %shame-link-base;
+
+  &:hover, &:focus {
+    text-decoration: underline !important;
+  }
+}

lms/static/sass/base/_variables.scss

@@ -326,6 +326,7 @@ $header-graphic-sub-color: $m-gray-d2;
 $error-color: $error-red;
 $warning-color: $m-pink;
 $confirm-color: $m-green;
+$active-color: $blue;
 
 // Notifications
 $notify-banner-bg-1: rgb(56,56,56);
@@ -441,3 +442,9 @@ $blue1: #4A90E2;
 $blue2: #00A1E5;
 $green1: #61A12E;
 $red1: #D0021B;
+
+// +feature: student notes
+// --------------------
+$student-notes-highlight-color-base: saturate($yellow, 50%);
+$student-notes-highlight-color: tint($student-notes-highlight-color-base, 60%);
+$student-notes-highlight-color-focus: $student-notes-highlight-color-base;