718471 - [shipping] UX overhaul in signoffs view, mostly diff

Status: RESOLVED WONTFIX

(Webtools Graveyard :: Elmo, defect, P3)

Description

[Axel Hecht [:Pike]]

Attachment: slider for diff, headers, table borders

So, my magically round circles don't work, I hear.

I've taken a stab at a working mockup today to replace that with a jquery.ui slider, and a dedicated link target. That's attached, and works against somewhat recent caches for the media files on the dev server. AKA, it's gonna break at some point, but right now, I just need a single html file, cool.

I also re-did the borders in the table, not really happy with those.

I stole the column headers from the new team view, do they improve stuff?

TODO:

Slider positioning is awkward, and the hittarget is currently just a square. I think if we use background gradient to make that a triangle, it'd be OK. I explicitly do NOT want text links there, that's already in the table. Also use a background that matches the theme (jquery.ui) and doesn't suck as much us ui-widget-header.

Comment 1

[Peter Bengtsson [:peterbe]]

Comment on attachment 588930 [details]
slider for diff, headers, table borders

Sorry, but I still don't understand how it works. What's the vertical rectangle on the right? It grows and shrinks when I pull the slider. (after a while and some luck I figured out that I can click that to open a diff URL based on the slider)

I'm liking how it's impossible to do a diff in the inverse time. e.g. the left of the diff is always an older changeset. 

What would maybe make it better is...

* the slider shouldn't be continuous (and also not a gradient of gray) and should snap only where there are changesets. 

* upon sliding into different changesets I'd rather have a normal hyperlink appear somewhere (perhaps near the "Diff" column header) that says 
``<a href="?from=AAA&to=BBB">View diff between <em>AAA</em> and <em>BBB</em></a>``

If this functionality is of importance we should simply drag in Jason again for his fresh thoughts. We can probably pin him down on Friday at the UX office hours.

BTW, the new column headers are wonderful! I like that a lot!

Comment 2

[Axel Hecht [:Pike]]

The snap stuff requires more html work than I wanted to put into the mockup. Basically, the offset magic between the diff slider and the td is hard because the offsetTop of the diff is actually the body, which the offsetParent of the td's are the table. I would also like to animate the snap-to-revision, but that's require more js still, as the plain .animate just does CSS length values.

As I said before, I'm pretty sure that a link with text isn't going to cut it either. I also know that the block of gray isn't a great hit target right now. I think if an html coder makes that block a triangle with the right background gradient so that it looks more arrow-ish, I'd expect it to make a much better target.

Comment 3

[Peter Bengtsson [:peterbe]]

Shall I make a request to Jason to help him come up with something simple and simple?
I think I will. We shouldn't be bikeshedding around UX when it's not our core strengths. Also, we mustn't dump too much time into this considering the few people who actually need it.

Comment 4

[Axel Hecht [:Pike]]

Sounds good to get more input.

I'm not convinced that few people need the diff view, I'd hope that eventually localizers would use it to review before signing off.

Comment 5

[Axel Hecht [:Pike]]

Peter, did you get a chance to talk to Jason?

Comment 6

[Peter Bengtsson [:peterbe]]

(In reply to Axel Hecht [:Pike] from comment #5)
> Peter, did you get a chance to talk to Jason?

We lost Jason off the webdev team. Then, he proceeded to leave Mozilla entirely :(
And now they don't run any UX office hours on Fridays. 

Let's bring up this discussion together with Laura next next Monday.

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 588930 [details]
slider for diff, headers, table borders

How are we doing these days wrt UX? Anyone to go to?

Comment 8

[Peter Bengtsson [:peterbe]]

We still don't have anybody in Webdev to go to for involvement in UX and usability and stuff. 

We can either try to take your prototype, improve on it with some of my comments and build something. 
Or we can start afresh with another prototype that isn't using a slider or some round blobs that you click. 

Whay say you? What do you prefer Axel?

Comment 9

[Axel Hecht [:Pike]]

I'd welcome proposals.

That said, I guess I prefer to start with something, compared to starting with nothing and no owner.

Comment 10

[Peter Bengtsson [:peterbe]]

I think my primary objection to the current UI is how unclear it is about clicking the little diffanchors. 

First of all, it's not clear that you're supposed to click them to open up the diff. 
Secondly, if you have dragged them wrong or you've dragged them so that you're actually looking to make a diff on the same changeset nothing happens. 

I'm going to see if I can build a prototype where a little tooltip appears, or a button, when it's possible to open a diff.

Comment 11

[Peter Bengtsson [:peterbe]]

Attachment: bug718471.diff

I think this addresses the user interface concerns I raised. 
The tooltip gives a clue A) what will be compared and B) that you can "click" the diffanchor to get the actual diff. 

I'll upload two screenshots too. My branch is here: https://github.com/peterbe/elmo/tree/feature/bug-718471-ux-overhaul-in-signoffs-view-mostly-diff

Comment 12

[Peter Bengtsson [:peterbe]]

Attachment: Screenshot 2013-12-11 17.23.26.png

when there's something to compare

Comment 13

[Peter Bengtsson [:peterbe]]

Attachment: Screenshot 2013-12-11 17.23.48.png

When there's nothing to compare.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 8346197 [details] [diff] [review]
bug718471.diff

Review of attachment 8346197 [details] [diff] [review]:
-----------------------------------------------------------------

The primary issue here is that no-one understand that there is a click target.

The click target is the complete right column, not the diff markers, for example.

I don't think that a tooltip over the anchors changes our situation really, thus r-. I don't think that a tooltip in general helps a lot, too.

Comment 15

[Laura Thomson :laura]

Schalk, can you help Peter with this? It needs UX love, badly.

Comment 16

[Schalk Neethling [:espressive]]

I surely can, let me read the details.

Comment 17

[Schalk Neethling [:espressive]]

So, just to be clear. The basic need here is to provide a way for users to select two different 'commits' and then open a diff view between the two, correct?

Just on a side note, currently when opening a diff I am getting:
'Settings' object has no attribute 'REPOSITORY_BASE'

Is there a specific setting I can use for this or, should I simply ignore it? Thanks!

Comment 18

[Peter Bengtsson [:peterbe]]

(In reply to Schalk Neethling [:espressive] from comment #17)
> So, just to be clear. The basic need here is to provide a way for users to
> select two different 'commits' and then open a diff view between the two,
> correct?
> 
Yes. 

Perhaps a place to start is https://bugzilla.mozilla.org/show_bug.cgi?id=718471#c14 and in particular, Pike's comment of: "The primary issue here is that no-one understand that there is a click target."



> Just on a side note, currently when opening a diff I am getting:
> 'Settings' object has no attribute 'REPOSITORY_BASE'
> 
> Is there a specific setting I can use for this or, should I simply ignore
> it? Thanks!

Ignore. Even if you have it configured you don't have the files needed. 

If you want to see some diffs for real, I suggest you try the same URL on l10n.mozilla.org.

Comment 19

[Schalk Neethling [:espressive]]

Got it, thanks Peter.

Comment 20

[Schalk Neethling [:espressive]]

Just wanted to update everyone, I have not forgotten about this. I am actually working on it at the moment.

Comment 21

[Schalk Neethling [:espressive]]

Attachment: bug718471-ux-overhaul-diff-portion.patch

Ok, so here is an idea for the diff view capability.

Couple of things though:

1) Currently the bar as at the top off the table but, we can also explore an option of fixing the bar to the bottom of the viewport and then, add some JS so that it does not overlap the footer as you scroll down.

2) The two select elements could probably be disabled text fields but, I kinda like the look of them being selects ;) Although with that said, one could possibly style a generic div in a nicer way, which I will definitely explore if the principal of this is accepted.

Other than that, I do not think there is anything else other than, let me know what you think.

Comment 22

[Axel Hecht [:Pike]]

Attachment: screenshot to have something to talk about.

Comment 23

[Axel Hecht [:Pike]]

Comment on attachment 8407544 [details] [diff] [review]
bug718471-ux-overhaul-diff-portion.patch

We've talked about this a bit, and I have a few comments:

What I like is the explicit "diff" button, together with visual clues as to what to click to change the diff.
The select being a select is OK, it'd be better though if that select wasn't empty.

There are also a few things that rubbed us backwards:

The default diff is gone. That's pretty useful in practice. One way to implement that would be to actually fill in the values in the selects.

The different markers for start and end. For one, we're generally really only interested in forward diffs, and they're making the page rather busy to read. The diff box could just read "Compare from ^[select box] to ^[select box] [View Diff]"

We need a visual indication in the pushes list of which versions are currently marked for comparison. When we're looking at diffs, the context of the version is more important that revision or commit message. Look at https://l10n.mozilla.org/source/pushes/gaia-l10n/ru, to get an idea ;-)

We'd like to have a UI element to quickly go to the diff from wherever you are on the page. It could be "magic" sauce, too, like just a click target that doesn't really have markup. I'm not sure that making the diff bar fixed or pseudo fixed is the answer. I think it's styling right now is OK, but wouldn't work if it overflowed real content.


Given the amount of UX feedback, I didn't actually look a the code yet, hope that's OK.

Comment 24

[Schalk Neethling [:espressive]]

(In reply to Axel Hecht [:Pike] from comment #23)
> Comment on attachment 8407544 [details] [diff] [review]
> bug718471-ux-overhaul-diff-portion.patch
> 
> We've talked about this a bit, and I have a few comments:
>
> Given the amount of UX feedback, I didn't actually look a the code yet, hope
> that's OK.

Thanks a bunch for the feedback. I will have another detailed read over it and revise the design to include the feedback. Get back to you ASAP

Comment 25

[Schalk Neethling [:espressive]]

(In reply to Axel Hecht [:Pike] from comment #23)
> Comment on attachment 8407544 [details] [diff] [review]
> bug718471-ux-overhaul-diff-portion.patch
> 
> We've talked about this a bit, and I have a few comments:
> 
> What I like is the explicit "diff" button, together with visual clues as to
> what to click to change the diff.
> The select being a select is OK, it'd be better though if that select wasn't
> empty.
> 
> There are also a few things that rubbed us backwards:
> 
> The default diff is gone.

Do you mean by that for example a page such as https://l10n.mozilla.org/shipping/signoffs/en-GB/fx30#7b47a2369a63 the default diffs would be 'Migrating aurora to beta for Firefox 30' and 'Axel Hecht accepted sign-off by Ian Neal on April 28, 2014 ' ?

Comment 26

[Schalk Neethling [:espressive]]

Hey Axel,

Quick question, what you prefer I use for the display value of the selects, the shotrev or the description i.e. fe501943bcb7 or more like 'Migrating aurora to beta for Firefox 30'?

Thanks!

Comment 27

[Axel Hecht [:Pike]]

McDreamy would like:

* a optional prefix (suggested), (rejected), (current), (pending) describing the sign-off state
* 4 char revision, I think that's good enough, and leaves room for 
* the comment of the commit (tip of the push by default)

The optional stuff might be good enough for a follow-up, though.

Comment 28

[Schalk Neethling [:espressive]]

Attachment: Screen Shot 2014-05-15 at 15.02.55.png

This screen shot shows a scenario where there is only one default revision set for the diff. The selected row is highlighted and the base revision is set to the shortrev

Comment 29

[Schalk Neethling [:espressive]]

Attachment: Screen Shot 2014-05-15 at 15.03.10.png

This screen shot shows a scenario where two default revisions exists. The selected rows are highlighted and the selects are set to the relevant shortrev values

Comment 30

[Schalk Neethling [:espressive]]

A couple of general comments based on the response in comment 27 and the screen shots uploaded in comment 28 and 29.

1) So current it uses the value of shortrev for the select's display value, point 1 in comment 27 suggests showing a status in front of the revision number. Is this value exposed the the UI?
2) Can I just strip the shortrev down to the first 4 chars?
3) Is this the description shows on each row, i.e something like 'Migrating aurora to beta for Firefox 30' or are you referring to a different value with regards to "comment of the commit"? If so, then same question as for point 1, is this currently exposed to the UI?

Thanks!

Comment 31

[Axel Hecht [:Pike]]

The current UI for selecting a diff is "drop the marker to the row". So all of revision, commit message, sign-off status are exposed as part of that. It also exposes the context in which that change is (earlier commits, later commits, what has been pushed together, and possibly why).
It also serves as UX for "while reading through the commits, I thought I should compare against this revision". Somewhat cumbersome at that, as you need to find a diff marker to drag over, but you can go back to the context.

Maybe to add more context, we rarely diff revision 15 against 29, i.e., we know the revisions we compare. Commonly, the revisions to compare are interesting because of their context in the process. The one from last time, the latest, the latest one that had a good run. Occasionally, I would compare "the latest on Fennec against the one I know is good on desktop Firefox", then I would remember the revision by hash, or just copy-n-paste.

Re 2), yes
Re 3), each row is a hg commit, and it shows the -m'update russian'. Loads of jargon, technically, it's Changeset.description. For a Sign-off, the corresponding push counts, for which taking the .tip is a good compromise. signoff.push.tip.description. (word of caution, .tip is a property hitting the database. cached, but still)

Comment 32

[Schalk Neethling [:espressive]]

Attachment: initial_load.png

Comment 33

[Schalk Neethling [:espressive]]

Attachment: next_steps.png

Comment 34

[Schalk Neethling [:espressive]]

Attachment: final_steps.png

Comment 35

[Axel Hecht [:Pike]]

This looks good, but a few comments:

I'd really like to drop the from/to distinction. It's one direction, just make those markers.
One thing I like in the original approach was that the visual thing in the row was the same thing as in the diff bar. Make all of those one single glyph, from, to, row markers? right now, there's like 4-6 glyphs involved, that's just too many, IMHO.

Comment 36

[Schalk Neethling [:espressive]]

(In reply to Axel Hecht [:Pike] from comment #35)
> This looks good, but a few comments:
> 

Quick question: Will fallback.repository.name and push.repo ever both be set? Or is it literally as the name suggests, the fallback will only be set if the other is empty?

These are in different places in the template so, I am just wondering:

https://github.com/mozilla/elmo/blob/master/apps/shipping/templates/shipping/signoffs.html#L148
https://github.com/mozilla/elmo/blob/master/apps/shipping/templates/shipping/signoff-rows.html#L85

Thanks!

Comment 37

[Axel Hecht [:Pike]]

fallback is an actual life.models.Push, whereas the entries in pushes are "kinda-like" push objects, created, mostly, at https://github.com/mozilla/elmo/blob/master/apps/shipping/views/signoff.py#L361, where 'repo' is set to push.repository.name.

The two exist rather independent of each other, and need slightly different handling in the template. Or you'd create a fake fake-push object for fallback, that could probably work, too.

Comment 38

[Schalk Neethling [:espressive]]

Hey Axel,

Please have a look at the screen cast I shared here: http://screencast.com/t/rhCJACln77NE

It still requires a couple of tweaks and some JS validation but, let me know what you think. Thanks!

Comment 39

[Axel Hecht [:Pike]]

That looks good. 'A' should be the lower one in the end, the changes are shown "newest on top, oldest at the end".

Also, I'd like to get a tad richer data into the drop down ala comment 27

Comment 40

[Schalk Neethling [:espressive]]

Hey Axel,

Ran into this situation where two diffbases are added to the same row. Looking at the code, it is clearly happening because on the following line, it uses a for:
https://github.com/mozilla/elmo/blob/master/apps/shipping/templates/shipping/signoff-rows.html#L38

but later on, it uses an if:
https://github.com/mozilla/elmo/blob/master/apps/shipping/templates/shipping/signoff-rows.html#L88

Should the first perhaps also be an if instead of a for loop?

If not, how do we distinguish the two so I can mark the one as 'A' and the other as 'B' respectively and, will we ever have more than two?

Comment 41

[Axel Hecht [:Pike]]

It's ligit that both diff markers are in the same row. One example is a localization with just no prior activity in terms of sign-offs, just a hand full of pushes.

We need to show both markers, and showing them on the last push is as good as anything else.

Note, the code to trigger the diff disables opening a new tab if it's requested to diff the same version against itself.

There are never any other number than two diff markers in the current code. Never less, never more.

Comment 42

[Schalk Neethling [:espressive]]

Attachment: bug718471-ux-overhaul-diff-portion.patch

Hey Axel,

Here is a new patch. Let me know what you think. The prepending of pending, accepted is missing in this patch, but that is a small addition.

Comment 43

[Schalk Neethling [:espressive]]

Oops, ignore that patch, it was done against master :/

Comment 44

[Schalk Neethling [:espressive]]

Attachment: bug718471-ux-overhaul-diff-portion.patch

Updated patch against the develop branch

Comment 45

[Axel Hecht [:Pike]]

Comment on attachment 8434003 [details] [diff] [review]
bug718471-ux-overhaul-diff-portion.patch

Review of attachment 8434003 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the lag.

What I like is the basic setup of the A and B marker, and the diff bar.

What I don't like is the changed behavior around dragging the markers, the markers seem to be jumping around, and the glyph in the diff button. The latter is also kinda bad licensing-wise, I suggest to drop that.

The A and B versions are still in the wrong order, the B should be on top of A, as we're comparing old vs new.

The descriptions in the selects should be modified to be a shorter revision, and have the description of the changeset.

I think some of the oddities around the drag behavior are coming from the full-circle that this patch took. I think it'd be cleaner to re-implement the marker and drag logic as a modification of the current code on develop than to find the culprits of the patched behavior. Make the markers be B and A instead of circles, make them switch if they get dragged past each other, add and hook up the diff bar.

I understand that that's annoying, are you up for that?

Comment 46

[Schalk Neethling [:espressive]]

(In reply to Axel Hecht [:Pike] from comment #45)
> Comment on attachment 8434003 [details] [diff] [review]
> bug718471-ux-overhaul-diff-portion.patch
> 
> Review of attachment 8434003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the lag.
> 
> What I like is the basic setup of the A and B marker, and the diff bar.
> 
> I understand that that's annoying, are you up for that?

Thanks for the feedback Axel, sure thing, I am up for that. Get back to you as soon as possible.

Comment 47

[Schalk Neethling [:espressive]]

Attachment: bug718471-ux-overhaul-diff-portion.patch

"What I don't like is the changed behavior around dragging the markers, the markers seem to be jumping around.."

This was because of a snapping behavior used to avoid users dropping the marker on an invalid position. I have found a much better way to handle this so, the jumping behavior no longer occur.

"and the glyph in the diff button."

Removed ;)

"The A and B versions are still in the wrong order, the B should be on top of A, as we're comparing old vs new."

Done

"The descriptions in the selects should be modified to be a shorter revision, and have the description of the changeset."

Done

"Make the markers be B and A instead of circles,"

I did not implement it like this because I have to disagree here. From a useability perspective just having an A and B in the rows will look more like labels and not element that can be dragged. This might lead the user to believe that the way to change these is to use the select, which when interacted with will cause even greater confusion.

What I have done instead is keep the A/B, make then slightly larger and add a small icon to further clarify the fact that these are draggable elements.

"change these is to use the select, which when interacted with will cause even greater confusion."

On this topic, I was not entirely happy with using the select drop downs from the start as I mentioned and so, I have dropped them in favor of something simpler, that I believe will also cause less user confusion.

"make them switch if they get dragged past each other..."

Here again I have to disagree and believe that this behavior will make the UI less useable and not more. I feel a user might feel confused if the handle they are dragging sudenly changes from one to the other during a drag operation. I have thus not implemented this behavior here.

With all of that said I believe the UI as it is now, is easy to use, simple to understand and effective. Let me know your thoughts, thanks!

Comment 48

[Schalk Neethling [:espressive]]

hmmmm, this patch does not look like he latest one. Let me create a new one and attach.

Comment 49

[Schalk Neethling [:espressive]]

Attachment: bug718471-diffview.patch

Correct patch file

Comment 50

[[DEACTIVATED] Adrian Gaudebert]

Comment on attachment 8447949 [details] [diff] [review]
bug718471-diffview.patch

Review of attachment 8447949 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Schalk, 

I went through this patch, played a bit with it and talked with Pike about it. Here are a few comments:

* I was not able to understand the interface easily. My main concerns were that I couldn't see that the A and B dots were actionable. I think we should have more visible markers. Axel likes the fact of having just a big white A in a black box. I thought that colors would be nice, but then that page already has a bunch of different colors (for special sign-offs for example) so it might be too much. I'm not really sure what would be best here, but I'd like to take another look when my next comments are addressed. 

* Could you please revert back to using select boxes for the diffs? I like the look of the current UI but it makes me think you can write something in the box and that is not the case. Plus Axel tells me that in the future we will want to load those select boxes with some default data to make it even more useful and easy to developers. 

* Also, it would be much better if you had the commit message in the top boxes, along with the short sha. Something like "8ee94 - Bug 991543 - Update desktop l10n search plugins with resultdomain" is much better to understand and process, especially so when we don't even show the sha anywhere on this page.

* I don't like the green background behind the markers. It doesn't make sense to me, and it's buggy (sometimes it's there, sometimes not, and I'm not sure why or what they mean). If it's meant to highlight a zone, I think a variation of gray would be better. This green color seems a bit out of place there to me. 

* When you move a marker, it is slightly shifted to the right, and it is duplicated. It works well on master, so I suppose that's a bug you introduced, sadly. :)

* The marker A should never be on a commit that is more recent than B. It would be good if you could automatically switch them whenever a user makes A more recent than B. I don't think any sweet effect is needed, just invert the two markers when the drop happens. 

* Can you make the top menu look like the one on the team page? For example: https://l10n.mozilla.org/teams/fr

In my next comment I'll include an screenshot of a tweaked version of your work. It doesn't have the select boxes but I think it's a bit more understandable than the current version. The fact that there are different colors for the markers is still debatable, and we would need to try that on a more advanced version of this, because it will change significantly. Anyway, I just didn't want that image to be wasted. :P

Comment 51

[[DEACTIVATED] Adrian Gaudebert]

Attachment: elmo-diff-ui.png

Little tweaks to Schalk's last patch. Just for reference.

Comment 52

[Schalk Neethling [:espressive]]

Thanks for all of the feedback Adrian, I have actually thought about this one a lot since the last patch but, because things were so quiet I thought perhaps the feature was not needed or, the project went with a different solution.

Let me do some rework, including your suggestions from above, and get back to you as soon as possible.

Comment 53

[Schalk Neethling [:espressive]]

Attachment: elmo_w001.png

Hey l10n folks! It ha been a while. I finally had time to think about this UX problem and I wish to here propose a simple solution which is clear, simple and I believe effective.

To speed up iterations from here on I have done this as a wireframe mockup so, please have a look and give me your feedback, whether those be scribbles and indications on the wireframe or comments in the bug.

Thank you!

Comment 54

[Axel Hecht [:Pike]]

I've though about this bug for a while, and I don't see us spending our time on converging to a solution.

I think we should all spend our time on things that will land, so I'm resolving this bug as WONTFIX.

I'm sorry for all the work that has gone past here, but I think that's the right solution considering the state and the end benefit.

Comment 55

[Schalk Neethling [:espressive]]

(In reply to Axel Hecht [:Pike] from comment #54)
> I've though about this bug for a while, and I don't see us spending our time
> on converging to a solution.
> 
> I think we should all spend our time on things that will land, so I'm
> resolving this bug as WONTFIX.
> 
> I'm sorry for all the work that has gone past here, but I think that's the
> right solution considering the state and the end benefit.

It is unfortunate that we did not come to a final solution that would work for everyone but, in the end I think you resolution is best for now. If at some point an idea should jump into my head, I will be sure to let you know.

Comment 56

[Axel Hecht [:Pike]]

(In reply to Axel Hecht [:Pike] from comment #54)
> I've though about this bug for a while, and I don't see us spending our time
> on converging to a solution.
> 
> I think we should all spend our time on things that will land, so I'm
> resolving this bug as WONTFIX.
> 
> I'm sorry for all the work that has gone past here, but I think that's the
> right solution considering the state and the end benefit.

Actually resolving now. The UI is also used by less people, now that l10n-drivers do sign-offs.