The header comment box in the 'Finish Review' dialog is confusing

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mdoglio, Assigned: davidwalsh)

Tracking

(Blocks: 1 bug)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
In the 'finish review' dialog it's not clear what the 'ok' and 'cancel' buttons will do when clicked. Those buttons are imho redundant and can mislead the user to think they are there to publish/discard the review.
Also, I would expect the cancel button to be a 'reset text' kind of thing, but instead it either revert the text to it's previous version or the dialog disappear completely and an unstyled link with text 'add header' is left. The overall experience is very confusing.
(Reporter)

Updated

3 years ago
Blocks: 1246611
Product: Developer Services → MozReview
i think we should:
- remove the "footer" completely
- remove the "edit header" label
- remove the "ok" and "cancel" buttons
- ensure hitting escape when the header textarea is focused doesn't close it
- increase the height of the textarea to 10em
Duplicate of this bug: 1278379

Comment 3

2 years ago
Note that the footer has since been removed.

Comment 4

2 years ago
The other thing we should do there is save a draft periodically, instead of only when an action (ok, finished edit, etc.) is done.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8810576 [details]
MozReview: Add autosave feature, update layout for dropdown dialog (Bug 1246617).

https://reviewboard.mozilla.org/r/92848/#review92824

At present what tis does is:

-  Starts the "Edit header" box open, even when there's an existing review draft
-  Hides the "OK" and "Cancel" buttons
-  Saves the comment every 5 seconds
-  Removes the edit icon -- the idea is we want this open at all times.

Issues to be addressed
-  Ugh, the "<p>" crap is showing up in the box again
-  In a very rare case, the textbox/editor disappears -- need to figure out how that's happening.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8810576 [details]
MozReview: Add autosave feature, update layout for dropdown dialog (Bug 1246617).

https://reviewboard.mozilla.org/r/92848/#review92922

::: reviewboard/reviewboard/static/rb/js/views/reviewDialogView_mozreview.js:1
(Diff revision 2)
>  (function() {

Need to brick the ESCAPE key to not close the editor.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8810576 [details]
MozReview: Add autosave feature, update layout for dropdown dialog (Bug 1246617).

https://reviewboard.mozilla.org/r/92848/#review92924

OK, this is good for a review!

Comment 14

2 years ago
mozreview-review
Comment on attachment 8810575 [details]
MozReview: Add appUtil, common css, and textEditorView forks for impending changes (Bug 1246617).

https://reviewboard.mozilla.org/r/92846/#review94460
Attachment #8810575 - Flags: review?(glob) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8810576 [details]
MozReview: Add autosave feature, update layout for dropdown dialog (Bug 1246617).

https://reviewboard.mozilla.org/r/92848/#review94462

great work - i'm really looking forward to seeing this on prod :)

::: reviewboard/reviewboard/static/rb/js/ui/views/textEditorView_mozreview.js:481
(Diff revision 3)
>      /*
>       * Hides the actual editor wrapper.
>       *
>       * The last value from the editor will be stored for later retrieval.
>       */
> -    _hideEditor: function() {
> +    _hideEditor: function(soft) {

instead of adding a 'soft' param here that disables the hiding of the editor (which is counter-intuitive considering the function name), please add a new method that just updates the stored value.

::: reviewboard/reviewboard/static/rb/js/views/draftReviewBannerView_mozreview.js:205
(Diff revision 3)
>        this._$dialogContainer.removeClass('show-dialog');
>  
>        // Save the partial review contents without publishing
>        if(this._RDVInstance) {
>          this._RDVInstance._saveReview(false);
> +        if(this._saveInterval) {

missing space after 'if'

::: reviewboard/reviewboard/static/rb/js/views/draftReviewBannerView_mozreview.js:232
(Diff revision 3)
> +
> +      this._saveInterval = setInterval(function() {
> +          if(_RDVInstance && _RDVInstance._bodyTopView.needsSave()) {
> +              _RDVInstance._saveReview(false, true);
> +          }
> +      }, 4000);

can we change this from every 4s to every 10s or longer?

the 'saving..' notice is slightly distracting, and 10s+ reduces the load on the server.

::: reviewboard/reviewboard/static/rb/js/views/reviewDialogView_mozreview.js:418
(Diff revision 3)
>          this.$editor = this.$('pre.reviewtext')
>              .inlineEditor(_.extend({
>                  cls: 'inline-comment-editor',
>                  editIconClass: 'rb-icon rb-icon-edit',
>                  notifyUnchangedCompletion: true,
> +                showButtons: false,

i love that these are gone now, but the left margin on the enable-markdown checkbox looks out of place now.

can you please align the enable-markdown with the left edge of the comments text area.

::: reviewboard/reviewboard/static/rb/js/views/reviewDialogView_mozreview.js:462
(Diff revision 3)
> +
> +        // Opening the editor first prevents HTML from displaying as
> +        // plain text within the editor
> +        this.openEditor();
> +        // Updating this option will prevent ESC from closing the editor
> +        this.$editor.inlineEditor('option', 'forceOpen', true);

when the dialog is initially opened, the comment text field is not focused.

::: reviewboard/reviewboard/static/rb/js/views/reviewDialogView_mozreview.js:1014
(Diff revision 3)
>          }, this);
>  
>          $.funcQueue('reviewForm').add(function() {
>              var reviewBanner = RB.DraftReviewBannerView.instance;
>  
> +            if(isProgressSave) return;

rather than enqueuing a fn that immediately returns if isProgressSave is true, it would be better to not queue the fn.
Attachment #8810576 - Flags: review?(glob) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8810576 [details]
MozReview: Add autosave feature, update layout for dropdown dialog (Bug 1246617).

https://reviewboard.mozilla.org/r/92848/#review95152

very close now; just a minor issue with the default position of the cursor.

::: reviewboard/reviewboard/static/rb/js/ui/views/textEditorView_mozreview.js:501
(Diff revisions 3 - 4)
> +    _updateStoredValue: function() {
> +        if (this._editor) {
> +            this._value = this._editor.getText();
> +            this._richTextDirty = false;
>          }
> -    }
> +    },

remove this trailing comma

::: reviewboard/reviewboard/static/rb/js/ui/views/textEditorView_mozreview.js:574
(Diff revisions 3 - 4)
>                      textEditor._hideEditor();
>                      textEditor.setRichText(origRichText);
>                  });
>  
>                  $editor.on('complete', function() {
> -                    textEditor._hideEditor(isProgressSave);
> +                    if(isProgressSave) {

missing space after 'if'

::: reviewboard/reviewboard/static/rb/js/views/reviewDialogView_mozreview.js:464
(Diff revisions 3 - 4)
>          // plain text within the editor
>          this.openEditor();
>          // Updating this option will prevent ESC from closing the editor
>          this.$editor.inlineEditor('option', 'forceOpen', true);
> +        // Ensures the editor gets focus upon open
> +        this.$editor.inlineEditor('showEditor', true);

this works, except the cursor is positioned at the start of the text field.

i found myself always having to move the cursor to the end of the field to append comments; let's do that when the field is given focus.
Attachment #8810576 - Flags: review?(glob) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

2 years ago
mozreview-review-reply
Comment on attachment 8810576 [details]
MozReview: Add autosave feature, update layout for dropdown dialog (Bug 1246617).

https://reviewboard.mozilla.org/r/92848/#review95152

> this works, except the cursor is positioned at the start of the text field.
> 
> i found myself always having to move the cursor to the end of the field to append comments; let's do that when the field is given focus.

This was existing behavior but I've found a way around it.

Comment 22

2 years ago
mozreview-review
Comment on attachment 8810576 [details]
MozReview: Add autosave feature, update layout for dropdown dialog (Bug 1246617).

https://reviewboard.mozilla.org/r/92848/#review95458

\o/
Attachment #8810576 - Flags: review?(glob) → review+

Comment 23

2 years ago
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/webtools/reviewboard/rev/b5ce4efa435e
MozReview: Add appUtil, common css, and textEditorView forks for impending changes . r=glob
https://hg.mozilla.org/webtools/reviewboard/rev/c9a33d308f43
MozReview: Add autosave feature, update layout for dropdown dialog . r=glob
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Assignee: nobody → dwalsh
You need to log in before you can comment on or make changes to this bug.