Page MenuHomePhabricator

Inline Image Commenting

Maniphest Tasks
Restricted Maniphest Task
Restricted Maniphest Task
T3612: Lightbox v2
Authored By
chad, Jul 25 2013
"Love" token, awarded by RodrigoPrior.

Mock History

Current Revision111

Mock Description

I took a step back while brushing up the design of Pholio to see if we can add a baseline image commenting system across all of Phabricator, with special views in Pholio, Differential, and Lightbox. Attention was paid to mobile as well. Overall I want designers included more in discussion around Phabricator and hope this brings both better lightweight and heavyweight discussion into more areas.

Event Timeline

Cool stuff!

Lightbox generally looks sweet. Not sure what the image-y icon does when you click it?

Instead of 1 up and 2 up for code reviews -- isn't it side by side vs vertical? (Not sure what "vertical" is called...) The 1 up isn't good for diffing as designed since you can't compare the two images in the actual view?

On reviews in particular, but generally, the up / down arrows don't feel intuitive to me. Left and right navigate through the set, so what do up and down do...? I get the idea is to navigate through _history_ of that specific image but this breaks down for me mentally, especially in the concept of a given diff which shows a specific atomic change from A -> B (even if there exists some earlier changeset that went from A -> C or whatever.) Also, what if you press down, then right or something in Differential or Pholio?

Inline Comments

What does this control do?

This direction makes a lot of sense to me. I especially like the full-screen part for the image. I'd consider putting the mock title back up top -or- diminishing its presence further somehow.

this stuff is pretty cool. I guess we'd need to save a new version of the image (what you cropped) to get this done? or is there a fancy transformation we should be doing in css / js?

  • I designed the inline crop to just use the same image already on the screen, background-positioned. Then we can overlay some alpha divs to achieve a crop'd look.
  • The little photo icon button is an image selector, so it'll toggle on and off a scrollable list of images on the task or mock. I think I just forgot the design it out for this set.
  • I can see uses for both 1-up and 2-up image review. New marketing images, like at Apple, I'd want fullscreen to really inspect the everything. Smaller logo changes I might want to see 2-up. I don't think 2-up is something we need immediately, but I think it fits in well with Differential and would be a key differentiator vs. other competitors.

-Up and down go up and down image "revisions". So in a Mock set, if I change out a single image, I can go back in time on that one. Think Apple Time Machine.

Cool, looking forward to seeing the image selecting UI.

Up / down -- I get what it does, it just doesn't make sense to me in Differential in particular.

For Differential, I think of a diff as a set of changes that moves the world from A -> B. You might make changes on changes, but you are ultimately looking at (approving) the change from A -> B. Bob-design-principle -- within the same tool, an "image diff viewer" should work almost exactly like the "code diff viewer" since you are reviewing data in both cases. Or in other words, the image review UI should be usable to review code snippets... Or in even other words, I feel like you are mocking a "Differential Focus Mode" that serves up one specific change at a time and should be capable of working for images as text...

So say you are looking at image foo becoming image bar in this change from A -> B. If you hit down you are moving to something where the world went from A -> C. This world might not have any changes to the image foo, so now should you see nothing? Or maybe still keep looking at foo and show <no changes> on the right? No idea in a one up view? This world might have _the same change_ you were just looking at from foo -> bar and thus feel very broken as you hit down and nothing changes visually...? This is also why I ask about what should happen if you hit down -> right; in theory to me you've CHANGED DIFFs by hitting down so right should keep you in that new A->C state no?

Does that make sense in terms of diff data? Basically when I think about using this interface it just seems confusing? I guess you could do something where you say "forget this whole diff thing... I'm going to look at the images independently and pretend I'm in a different tool for a minute" but I think its pretty bad to have two separate mental models in the same tool for reviewing data. I'd rather have the proper set of images populated based on what "diff of diffs" the user chooses in that element and get rid of any sort of changing what diff you're looking at on the fly from inside the image viewer stuff.

More generally, I think its confusing to know what you're looking at in any given moment with this navigation. Suppose I hit down and go get a cup of coffee - what in the UI tells me on return I am not looking at the latest version and instead something older? Pholio's concepts are cleaner for this -- unlike a revision, there aren't many changes that are atomic and instead images can be tweaked individually -- but it still gets pretty confusing pretty fast when you're navigating around images with slight tweaks.

I also suspect that looking at history is super, duper rare, and that making history is super, duper rare. These are just hunches without the tool shipping and getting usage.

Basically, I recommend always showing the "latest" and having links to "image history" if folks want to see that, which lays out the full image history _for just that image_ so you can see all N changes at once, and then doing this just for Pholio. Note this is how D6560 implements it (without touching the lightbox) with the big caveat of the full image history view is just the existing mock view with the right data jammed into it and needs significant work.

So in Differential I was thinking up/down would go up and down the diff's images, and left and right would toggle old and new, but I think you're right that this isn't in keeping with what we have everywhere else and can be confusing.

How about we simplify it to left right cycles through images as usual, no up/down, and we always show 2-up if it's replacement, or 1-up if it's just a new image? This is basically just a lightbox for Differential.

Oh hrm, I guess we already have a lightbox view in Differential ala comments. That might get confusing. Let me think about that.

chad changed the visibility from "All Users" to "Pholio (Project)".Sep 17 2013, 5:02 PM
chad changed the visibility from "Pholio (Project)" to "Public (No Login Required)".Jul 1 2016, 9:47 PM