Closed Bug 1294323 Opened 8 years ago Closed 8 years ago

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

Categories

(MozReview Graveyard :: Review Board: User Interface, defect, P1)

Product:
MozReview Graveyard
Component:
Review Board: User Interface
Version:
Production
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mozbugz, Assigned: davidwalsh)

References

Details

Attachments

(2 files)

MozReview: Move status flag dropdown to review bar (Bug 1294323).
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Move status flag dropdown to review bar (Bug 1294323).
58 bytes, text/x-review-board-request
Details 
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.)
Assignee: nobody → dwalsh
Priority: -- → P1
Attachment #8794402 - Flags: review?(smacleod) → review?(glob)
Attachment #8794403 - Flags: review?(smacleod) → review?(glob)
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 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 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*
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 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 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 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 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 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 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 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 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+
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
Closed: 8 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: