"Publish" button should show currently-selected review flag, or offer option

RESOLVED FIXED

Status

MozReview
Review Board: User Interface
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gerald, Assigned: davidwalsh)

Tracking

Production

Details

MozReview Requests

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

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Once a review comment has been added, a green bar appears at the top with:
> You have a pending review. [Edit Review v][Publish][Discard]

This "Publish" button is a bit dangerous, as it doesn't show which review flag is currently selected.

At least I think it should show that flag, e.g. [Publish (r+)].

Or possibly better, mozreview could provide a one-click way to select the review flag and publish, e.g.: [Publish (remove r?)][Publish (r?)][Publish (r+)][Publish (r-)].
And all these buttons would collapse into a single [Publish (r?)] (please keep the flag in it though) when the review editor is open, because the review flag is a drop-down inside the editor in this case -- Unless you remove that drop-down altogether.
But this might look a bit ugly. Time for a focus group!
I like the idea of putting the dropdown into the banner, although if you have no diff comments and are going to "Finish Review" to leave, say, an r+, I wonder if the dropdown will be noticeable if it's only in the banner.... Hm well that's where you have to go to hit "Publish" so I guess it would become apparent pretty quickly.  Your idea (if I understand it correctly) of basically moving the dropdown from the banner to the dialog once you hit "Edit Review" would work too but might be more confusing (UI elements moving around is probably not super friendly).

Definitely worth experimenting with, though, as I agree that the current requirement to open Edit Review to set the flag is not ideal.

(Also this touches on the weirdness that, if you leave diff comments, you get a banner that makes it fairly clear what to do next to submit your review.  But if you just want to set an r+ (or any other flag) without comments, then you have to know to click "Finish Review".  I'd love to somehow unify these two flows, or at least make the latter more obvious.  But that's for another bug.)

Updated

a year ago
Duplicate of this bug: 1301097

Updated

a year ago
Assignee: nobody → dwalsh
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

a year ago
Duplicate of this bug: 1308531
Attachment #8794402 - Flags: review?(smacleod) → review?(glob)
Attachment #8794403 - Flags: review?(smacleod) → review?(glob)

Comment 6

a year ago
mozreview-review
Comment on attachment 8794403 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80886/#review84024

looks good

::: reviewboard/reviewboard/static/rb/js/views/draftReviewBannerView_mozreview.js:74
(Diff revision 1)
>  
> +    addHooks: function() {
> +        /*
> +         * MozReview: Show review flag dropdown
> +         */
> +        if(this._hookViews.length != 0) return;

nit: missing space after if
Attachment #8794403 - Flags: review?(glob) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review84022

this doesn't work in all situations:
1. click 'finish review'
2. change flag to 'r+'
3. click 'finished edit'
4. click 'publish'
nothing happens.  then:
5. change flag to 'r?'
6. change flag to 'r-'
review is immediately published (without clicking on 'publish')

::: pylib/mozreview/mozreview/static/mozreview/js/review_flag.js:25
(Diff revision 1)
>     */
>    states: [' ', 'r?', 'r+', 'r-'],
>  
>    template: _.template([
> -      '<label for="mr-review-flag">Review state:</label> ',
> -      '<select id="mr-review-flag">',
> +      '<label for="mr-review-flag" style="display: none;">Review state:</label> ',
> +      '<select id="mr-review-flag" style="display: inline-block;">',

move the display:inline-block to the stylesheet

::: pylib/mozreview/mozreview/static/mozreview/js/review_flag.js:112
(Diff revision 1)
> +RB.DraftDialogHook = RB.ExtensionHook.extend({
> +    hookPoint: new RB.ExtensionHookPoint(),
> +
> +    defaults: _.defaults({
> +        viewType: null
> +    }, RB.ExtensionHook.prototype.defaults),
> +
> +    setUpHook: function() {
> +        console.assert(this.get('viewType'),
> +                       'DraftDialogHook instance does not have a ' +
> +                       '"viewType" attribute set.');
> +    }
> +});

2 spaces for indentation, not 4
Attachment #8794402 - Flags: review?(glob) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review84022

> move the display:inline-block to the stylesheet

...this is what happens when you're so full of glee it works and you forget, you know, the code.  *facepalm*
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review84022

> ...this is what happens when you're so full of glee it works and you forget, you know, the code.  *facepalm*

This also happens to be the commit I rebased on the ancient one.  #whenitrainsitpours

Comment 11

a year ago
mozreview-review
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review84576

Clearing the review flag here, :glob is going to be doing the reviews on this, can you swap us in the commit messages
Attachment #8794403 - Flags: review?(smacleod)

Comment 12

a year ago
mozreview-review
Comment on attachment 8794403 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80886/#review84578

Can you replace my nick with globs in the commit message, he's going to be doing the reviews.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review86130

looks like the issues aren't yet resolved; putting r- back.
Attachment #8794402 - Flags: review?(glob) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 17

a year ago
mozreview-review
Comment on attachment 8794403 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80886/#review86720

Glob:  I made changes here to fix (https://reviewboard.mozilla.org/r/80882/) so please update and r+ again! :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a year ago
mozreview-review
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review87084
Attachment #8794402 - Flags: review?(glob) → review+

Comment 21

a year ago
mozreview-review
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review87086
Attachment #8794402 - Flags: review+

Comment 22

a year ago
mozreview-review
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review87258

following on from our irc discussion where i was still able to reproduce the publish failure.
i triple checked that the latest revision is correctly applied, it is.

what i'm seeing when debugging is the 'sync' handler is bound with backbone.once in HeaderFooterCommentView.save, however the success callback isn't being triggered (maybe 'sync' isn't being triggered), so actions in the funcQueue aren't processed immediately.  when the sync event is triggered by other actions (ie. step 6 in my STR), the publish happens immediately.

can you try again to figure this out?  if you still can't reproduce or are otherwise stuck i'm ok with splitting that issue into a separate bug so this can land.
Attachment #8794402 - Flags: review-

Comment 23

a year ago
mozreview-review
Comment on attachment 8794402 [details]
MozReview: Move status flag dropdown to review bar (Bug 1294323).

https://reviewboard.mozilla.org/r/80882/#review88812

sorry - this was totally my mistake; i didn't update my repo correctly and somehow missed that when checking each time :(

you have indeed fixed the publish bug.
Attachment #8794402 - Flags: review- → review+

Comment 24

a year ago
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/webtools/reviewboard/rev/b81bf3718ac5
MozReview: Move status flag dropdown to review bar . r=glob
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Comment 25

a year ago
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/36782afd7994
MozReview: Move status flag dropdown to review bar . r=glob
You need to log in before you can comment on or make changes to this bug.