Improve styling for the review modal and when editing articles/metadata & Make kbox responsive to window resizing and overflow

kitsune/sumo/static/sumo/js/kbox.js

@@ -66,6 +66,8 @@
  *          Override the template to use for creating the modal.
  *      title:
  *          The kbox's title.
+ *      windowMargin:
+ *          Minimum margin between the kbox and the edge of the window.
  */
 
 var TEMPLATE = (
@@ -105,7 +107,9 @@ KBox.prototype = {
       preOpen: false,
       preClose: false,
       template: TEMPLATE,
-      title: self.$el.attr('title') || self.$el.attr('data-title')
+      title: self.$el.attr('title') || self.$el.attr('data-title'),
+      windowMargin: self.$el.data('viewport-margin') === undefined ?
+        20 : parseInt(self.$el.data('viewport-margin'), 10),
     }, options);
     self.options = options;
     self.$clickTarget = options.clickTarget && $(options.clickTarget);
@@ -187,7 +191,9 @@ KBox.prototype = {
       self.render();
     }
     self.$kbox.addClass('kbox-open');
+    self.handleOverflow();
     self.setPosition();
+    self.addResizeHandler();
     if (self.options.modal) {
       self.createOverlay();
     }
@@ -215,8 +221,27 @@ KBox.prototype = {
       }, 0);
     }
   },
+  addResizeHandler: function () {
+    var self = this;
+    // If position is none, return early instead of adding a listener that will never do anything
+    if (self.options.position === 'none') {
+      return;
+    }
+    self.resizeHandler = function () {
+      self.setPosition();
+    };
+    $(window).on('resize', self.resizeHandler);
+  },
+  removeResizeHandler: function () {
+    var self = this;
+    if (self.resizeHandler) {
+      $(window).off('resize', self.resizeHandler);
+      delete self.resizeHandler;
+    }
+  },
   setPosition: function (position) {
     var self = this;
+    const minMargin = self.options.windowMargin;
     if (!position) {
       position = self.options.position;
     }
@@ -226,18 +251,72 @@ KBox.prototype = {
     if (position === 'center') {
       let windowWidth = $(window).width();
       let windowHeight = $(window).height();
+
+      // Reset height and width limitations to get the actual initial kbox size
+      self.resetOverflow();
+
       let modalWidth = self.$kbox.outerWidth();
       let modalHeight = self.$kbox.outerHeight();
 
+      let left = Math.max((windowWidth - modalWidth) / 2, minMargin);
+      let top = Math.max((windowHeight - modalHeight) / 2, minMargin);
+
       self.$kbox.css({
-        'left': (windowWidth - modalWidth) / 2,
-        'top': (windowHeight - modalHeight) / 2,
+        'left': left,
+        'top': top,
         'right': 'inherit',
         'bottom': 'inherit',
         'position': 'fixed'
       });
+
+      self.handleOverflow();
+    }
+  },
+  handleOverflow: function () {
+    var self = this;
+    const minMargin = self.options.windowMargin;
+
+    let windowWidth = $(window).width();
+    let windowHeight = $(window).height();
+    let modalWidth = self.$kbox.outerWidth();
+    let modalHeight = self.$kbox.outerHeight();
+    let rect = self.$kbox[0].getBoundingClientRect();
+
+    if (rect.right > windowWidth - minMargin) {
+      self.$kbox.css({
+        'max-width': windowWidth - minMargin - rect.left
+      });
+    }
+    else if (rect.right < windowWidth - minMargin) {
+      self.$kbox.css({
+        'max-width': ''
+      });
+    }
+
+    // Due to max-width, kbox height may have changed.
+    rect = self.$kbox[0].getBoundingClientRect();
+
+    if (rect.bottom > windowHeight) {
+      self.$kbox.css({
+        'max-height': windowHeight - minMargin - rect.top,
+        'overflow-y': 'auto'
+      });
+    }
+    else if (rect.bottom < windowHeight - minMargin) {
+      self.$kbox.css({
+        'max-height': '',
+        'overflow-y': ''
+      });
     }
   },
+  resetOverflow: function () {
+    var self = this;
+    self.$kbox.css({
+      'max-width': '',
+      'max-height': '',
+      'overflow-y': ''
+    });
+  },
   close: function () {
     var self = this;
     if (self.options.preClose && !self.options.preClose.call(self)) {
@@ -250,6 +329,7 @@ KBox.prototype = {
     }
     self.isOpen = false;
     self.$kbox.removeClass('kbox-open');
+    self.removeResizeHandler();
     if (self.options.modal) {
       self.destroyOverlay();
     }
@@ -266,6 +346,7 @@ KBox.prototype = {
   destroy: function () {
     // return DOM to how it was originally, if possible.
     var self = this;
+    self.removeResizeHandler();
     if (self.$container && self.$ph) {
       self.$ph.replaceWith(self.$el.detach());
     }

kitsune/sumo/static/sumo/js/wiki.js

@@ -443,7 +443,8 @@ import collapsibleAccordionInit from "sumo/js/protocol-details-init";
     // when checked.
     var $checkbox = $('#id_needs_change'),
       $comment = $('#id_needs_change_comment'),
-      $commentlabel = $('label[for="id_needs_change_comment"]');
+      $commentlabel = $('label[for="id_needs_change_comment"]'),
+      $kbox = $checkbox.closest('.kbox').data('kbox');
 
     if ($checkbox.length > 0) {
       updateComment();
@@ -460,6 +461,11 @@ import collapsibleAccordionInit from "sumo/js/protocol-details-init";
         $comment.hide();
         $comment.find('textarea').prop('required', false);
       }
+      if ($kbox && $kbox.isOpen) {
+        $comment.add($commentlabel).promise().done(function () {
+          $kbox.handleOverflow();
+        });
+      }
     }
   }
 

kitsune/sumo/static/sumo/scss/base/forms/_fields.scss

@@ -128,10 +128,6 @@ label {
 
   &.full-width {
     max-width: 100%;
-
-    select {
-      max-width: 100%;
-    }
   }
 
   &.has-error,

kitsune/sumo/static/sumo/scss/components/_wiki.scss

@@ -637,6 +637,15 @@ article {
   form {
     padding: 0 0 20px;
   }
+
+  label[for="id_needs_change_comment"] {
+    font-weight: normal;
+  }
+
+  /* Remove margin after the needs-change field before the comment (when it's displayed). */
+  div:has(label[for="id_needs_change"]):has(+ div label[for="id_needs_change_comment"]) {
+    margin-bottom: 0;
+  }
 }
 
 #preview {
@@ -904,11 +913,7 @@ body.review {
 }
 
 #approve-modal {
-  p {
-    margin: 0 0 10px;
-  }
-
-  label {
+  .field label {
     font-weight: normal;
   }
 
@@ -950,11 +955,6 @@ body.review {
   div.needs-change {
     margin: 10px 0;
   }
-
-  textarea {
-    height: 100px;
-    width: 350px;
-  }
 }
 
 .html-rtl {
@@ -979,9 +979,13 @@ body.review {
   }
 }
 
+#self-approve-warning {
+    margin-top: 0;
+}
+
 .needs-change {
   .comment {
-    margin: 0 0 0 17px;
+    @include p.bidi(((margin, 0 0 0 calc(20px + #{p.$spacing-md}), 0 calc(20px + #{p.$spacing-md}) 0 0),));
 
     label {
       display: block;

kitsune/wiki/jinja2/wiki/edit.html

@@ -45,7 +45,7 @@ <h1 class="sumo-page-heading">{{ _('<em>Editing Content For:</em><br>{title}')|f
       <form action="" method="post">
         {% csrf_token %}
           {% for field in revision_form.visible_fields() %}
-            <div class="field {% if field.name == 'content' %}has-large-textarea{% endif %}">
+            <div class="field {% if field.name == 'content' %}has-large-textarea{% else %}full-width{% endif %}">
               {% if field.name not in ['comment', 'content'] %}
                 {% if not field.name == 'keywords' or document.allows(user, 'edit_keywords') %}
                   {{ field|label_with_help }}{{ field|safe }}

kitsune/wiki/jinja2/wiki/edit_metadata.html

@@ -34,7 +34,7 @@ <h1 class="sumo-page-heading">{{ _('<em>Editing Metadata For:</em><br>{title}')|
           <form action="" method="post" data-json-url="{{ url('wiki.json') }}" data-document-id="{{ document.id }}">
             {% csrf_token %}
             {% if in_staff_group(request.user) %}
-              <div class="mzp-c-emphasis-box field has-large-textarea">
+              <div class="mzp-c-emphasis-box field full-width">
                 <fieldset>
                     <legend>{{ _("Restrict Visibility") }}</legend>
                     <span class="{% if document_form.restrict_to_groups.errors %}has-error{% endif %}">
@@ -48,15 +48,23 @@ <h1 class="sumo-page-heading">{{ _('<em>Editing Metadata For:</em><br>{title}')|
               {% for field in document_form.visible_fields() if
                 (field.name != 'restrict_to_groups') and (field.name != 'is_localizable' or not document.translations.exists()) %}
                 {% if (field.name != 'related_documents') or document.allows_related_documents %}
-                <div class="field has-large-textarea">
+                <div class="field full-width">
                   {{ field|label_with_help }}
                   {% if field.name in ['products', 'topics'] %}
                     <span id="relevant-{{ field.name }}-disabled-message" hidden>{{ _("Disabled " + field.name + " are not available given the selections") }}</span>
                     <img id="relevant-{{ field.name }}-selected-checkmark" class="relevant-{{ field.name }}-selected" src="{{ webpack_static('sumo/img/green-checkmark.svg') }}" alt="checkmark" hidden/>
                     <span id="relevant-{{ field.name }}-selected-message" class="relevant-{{ field.name }}-selected" hidden></span>
                     <a href="#" id="relevant-{{ field.name }}-clear-selected" class="relevant-{{ field.name }}-selected" hidden>{{ _("Clear") }}</a>
                   {% endif %}
-                  {{ field }}
+                  {% if field.field.widget.input_type == 'checkbox' %}
+                    <div class="checkbox">
+                      {{ field }}
+                      {# Out current implementation of the custom checkmark is tied to label, so let's create an empty one. #}
+                      <label for="{{ field.id_for_label }}"></label>
+                    </div>
+                  {% else %}
+                    {{ field }}
+                  {% endif %}
                 </div>
                 {% endif %}
               {% endfor %}

kitsune/wiki/jinja2/wiki/includes/review_form.html

@@ -20,7 +20,7 @@
       {% if self_approving and document.locale == settings.WIKI_DEFAULT_LANGUAGE %}
         {# Only show self approving warning to en-US reviewers. In other locales, it
            is usual for the editor and the reviewer to be the same user. #}
-        <div class="mzp-c-notification-bar mzp-t-warning">
+        <div id="self-approve-warning" class="mzp-c-notification-bar mzp-t-warning">
           <div class="warning">
           {% trans url=url('groups.profile', 'reviewers') %}You are about to approve your own edit.
           Please consider asking our <a href="{{ url }}">reviewers</a> instead.{% endtrans %}
@@ -31,7 +31,15 @@
         <p>
           {% trans %}Clicking Accept will make this version of the article live! Please take a moment to be sure all options are correct.{% endtrans %}
         </p>
-        {{ form.significance }}
+        {# Mock form.significance to get the right styling. #}
+        <div id="{{ form.significance.auto_id }}">
+          {% for radio in form.significance %}
+            <div class="field radio is-condensed">
+              {{ radio.tag()|safe }}
+              <label for="{{ radio.id_for_label }}">{{ radio.choice_label }}</label>
+            </div>
+          {% endfor %}
+        </div>
       {% endif %}
       {% if not should_ask_significance and not self_approving %}
         <p>
@@ -51,17 +59,17 @@
         </div>
       {% endif %}
       {% if document.allows(user, 'mark_ready_for_l10n') %}
-        <div class="ready-for-l10n">
-          <label>
+        <div class="field checkbox ready-for-l10n">
             {{ form.is_ready_for_localization }}
-            {{ form.is_ready_for_localization.label }}
-          </label>
+            {{ form.is_ready_for_localization|label_with_help }}
         </div>
       {% endif %}
       {% if document.allows(user, 'edit_needs_change') %}
-        <div class="needs-change">
-          {{ form.needs_change }}
-          {{ form.needs_change|label_with_help }}
+        <div class="field needs-change">
+          <div class="checkbox">
+            {{ form.needs_change }}
+            {{ form.needs_change|label_with_help }}
+          </div>
           <div class="comment">
             {{ form.needs_change_comment|label_with_help }}{{ form.needs_change_comment }}
           </div>

kitsune/wiki/jinja2/wiki/new_document.html

@@ -22,10 +22,10 @@ <h1 class="sumo-page-heading">{{ _('Create a New Knowledge Base Article') }}</h1
           {% csrf_token %}
           <ul>
             {% if in_staff_group(request.user) %}
-            <li class="mzp-c-emphasis-box field has-large-textarea">
+            <li class="mzp-c-emphasis-box field full-width">
               <fieldset>
                   <legend>{{ _("Restrict Visibility") }}</legend>
-                  <span class="field has-large-textarea {% if document_form.restrict_to_groups.errors %}has-error{% endif %}">
+                  <span class="{% if document_form.restrict_to_groups.errors %}has-error{% endif %}">
                     {{ document_form.restrict_to_groups|label_with_help }}
                     {{ document_form.restrict_to_groups }}
                     {{ document_form.restrict_to_groups.errors }}
@@ -36,7 +36,7 @@ <h1 class="sumo-page-heading">{{ _('Create a New Knowledge Base Article') }}</h1
             {% for field in document_form.visible_fields() if (field.name != 'restrict_to_groups') and (field.name != 'is_localizable' or settings.WIKI_DEFAULT_LANGUAGE == request.LANGUAGE_CODE) %}
               <li class="
                   field
-                  has-large-textarea
+                  full-width
                   {% if field.name in ['allow_discussion', 'is_localizable'] %}checkbox{% endif %}
                   {% if field.errors %}has-error{% endif %}">
                 {# manually reorder checkboxes #}
@@ -59,7 +59,7 @@ <h1 class="sumo-page-heading">{{ _('Create a New Knowledge Base Article') }}</h1
           </ul>
           <ul>
             {% for field in revision_form.visible_fields() %}
-              <li class="field has-large-textarea {% if field.errors %}has-error{% endif %}">
+              <li class="field {% if field.name == 'content' %}has-large-textarea{% else %}full-width{% endif %} {% if field.errors %}has-error{% endif %}">
                 {% if field.name not in ['comment', 'content'] %}
                   {{ field|label_with_help }}{{ field|safe }}
                 {% elif field.name == 'content' %}