1248013 - Opening the "Finish Review" link in a new tab or window is broken

Status: RESOLVED INVALID

(MozReview Graveyard :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

When I context-click on "Finish Review" and select "Open Link in New Tab" or "Open Link in New Window", the thing that gets opened is not the "Finish Review" view.  Looks like we're using a link with a bogus href instead of an actual link, and that breaks all sorts of UI interactions.  :(

Comment 1

[Mark Côté [:mcote]]

That's because it's actually a dialog, not a separate page (which is totally not obvious atm, since it looks like a tab, but we're going to fix that).  This is, I think, a not-uncommon UI element in web apps... is this considered a bad practice in general?

Comment 2

[Boris Zbarsky [:bzbarsky]]

It's a not-uncommon UI element, but generally the thing leading to it doesn't try really hard to act like a link (show hand cursor, show link target in bottom left of window, etc).  Links should act like links; if someone wants a thing that opens a dialog, they use a button.

That said, my use case for wanting to open in new window was to view the diff and the "finish review" thing side by side so that I could type some general review comments while looking at the diff.  I made do with copy/paste from the URL bar so as to open the review in two windows and then going to "finish review" in one of them and the diff in another, but it took more time than if "open in new window" just worked, obviously.  So a dialog may not be the right UI element here, since it's presupposing a workflow ("look at the diff, then finish review without looking at the diff") that doesn't necessarily make sense...

Comment 3

[Mark Côté [:mcote]]

Chatting more with bz, sicking, dbaron, and jcranmer on IRC, I think this problem might boil down to the fact that the diff view should look more like the Finish Review dialog.  People like that the Finish Review dialog diff comments are inline (qv complaints about the comments collapsing into small bubbles on the left after being submitted), like they are in the Reviews view, and that there are places for general comments.  We should experiment with this idea.

Comment 4

[Mark Côté [:mcote]]

Related: glandium had also mentioned on dev.platform that the comment UX being different in the Reviews and Diffs views is confusing.  I agree; the inconsistency is jarring, at the least.  A smart fix to this problem could both make the UI more consistent and remove some unnecessary modality.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fwiw, I do still think that an easy way to open the thing I'm looking at again in as separate window is handy.  My use case of looking at the diff and commenting on the patch in general at the same time fails in a single-window view once the diff is longer than a single browser window....

Comment 6

[Steven MacLeod [:smacleod]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Fwiw, I do still think that an easy way to open the thing I'm looking at
> again in as separate window is handy.  My use case of looking at the diff
> and commenting on the patch in general at the same time fails in a
> single-window view once the diff is longer than a single browser window....

Review Board 2.6 (release date unannounced) provides a way to make general comments (which may open issues) similar to diff comments, just without being tied to particular code. Would that be sufficient for your use case?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Make such comments from the diff view?  If so, that would be sufficient for my use case, yes.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er, that is, it would be sufficient if I can continue seeing the diff while typing the comment, and can scroll in the diff view (and possibly load other parts of it, for cases when the diff view gets split across multiple pages) as needed while typing the comment.

Comment 9

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.