I'd probably take another pass at the UI / Colors, but I like the UX of it. Other ideas thought about but not mocked:

• Dropdown next to '2/5 Comments' that shows a snippet of the 3 undone comments.
• Dropdown next to '1 Draft' likewise could show you a snippet of the drafts you've written.

Also, why can't I add Subscribers through this comment interface? Not on EditEngine?

Yeah, not on EditEngine.

This is generally easy to implement.

Settings: I plan to implement the "Settings" menu at a minimum, with additional options ("Hide All", "Hide Done", etc) for T8909.

Drafts: If we throw away the minimap, I think putting "unsubmitted drafts" in the header is a good idea. If we keep the minimap, I'd be more take-it-or-leave-it depending on how much of a sense we have that the minimap is solving the problem. Note that there are really two problems:

• Users forgetting about comments they are currently editing (on the map: ).
• Users forgetting about unsubmitted drafts (on the map: ).

I think we were actually in a fairly good spot on the "unsubmitted drafts" problem already, before these changes (that is, no one had complained for a while, after the last round of "make it really obvious" UI changes).

However, these changes have almost certainly made the "you are currently editing stuff" problems worse than they already were, since you can now have multiple editors open simultaneously. That might not be a big deal or a very large effect, of course. I don't have a great sense of the absolute magnitude of this problem.

We could solve both these problems with two different elements:

[ 3 Comments Being Edited ] [ 7 Unsubmitted Drafts ] [ 4 / 11 Comments ] ...

...but this starts to feel pretty clumsy to me.

3 / 5 Comments: I vaguely don't like that this (a) gives users an implicit progress bar and (b) teaches new users that they should be checking off every comment. This element may also be a little misleading/confusing for non-author users, who can not mark comments done, and have no way to affect the "3". It's probably not too useful for them to jump to not-done comments. If this says "5 / 5 comments", does it do nothing when clicked? Or cycle you through all comments?

Up / Down: If you don't have a comment selected already, we probably can't make these work super intuitively: there might be a lot of stuff on screen, and the user's sense of what they expect the button to do may be different than our best guess. They are easy to implement if you have a comment selected.

Progress Bar: I can barely see this on my monitor, and I'm not sure what it's showing progress of. If it's "marking comments done", I oppose building/shipping it. I really don't want to ship features that push review in the direction of "mechanically fill up a progress bar".

I think this stuff is mostly fine. I generally think the minimap is a better/more powerful solution to these problems because it's spatial and lets you do things like skip over a thread and return to it, discourages big diffs, and discourages thinking about changes as progress bars that you fill up. But it's definitely not as clean looking or as easy to understand at a glance. If you prefer this direction I'm happy to toss the minimap and implement this instead.

How does Pholio manage to not send HTML email? That's quite impressive!

I'll do another photoshop pass here with those thoughts

@epriestley - pholio needs some EditEngine love 😉

To be clear, the "progress bar" background was a simple scroll indicator, nothing related to marking things done. Some blogs use them and I find in ... interesting ... nothing more. It's sort of nice to know how much you have to read through to be finished. It would start at the top of the first file and end when the final comment box appears. You can see one in action here: http://www.avclub.com/article/why-does-superman-keep-getting-turned-dictator-255977

(it's probably distracting and pointless though for code review)

(though we could add a mini-minimap to it, just a thought)

Took another pass here. I'd still like to figure out a way to get and working, but I could also just design them into the comments like they previously were. I think it's better though to have them in the top bar, since I could click on the and get right to the first bit of work I need to do.

The first two images seem to have been lost, Pholio bug?

What I'm seeing:

Safari only

What I'm seeing, in Chrome?

oh, maybe file permissions then

I tried logged out Safari and saw no images

Probably permissions, let me see if I can reproduce locally. I thought I tested that but maybe didn't get the "replace an image" transaction specifically.

It's still probably Safari though

I deployed D18042 to secure, but it isn't retroactive. You can either:

• find the Fxxx IDs of those two images and {Fxxx} them into a comment. That will attach them and fix the main UI; or
• upload them again (which will also test D18042 for correctness).

Have you tried Chrome?

Oh! Works great in Chrome.

Replacing the whole UI with a question mark is bold, but I like it! Gimmie a sec and I'll shoot you a diff.

Cool, Safari seems fixed again.

Okay, I'm going to implement these behaviors:

• Completely remove scrollbar objectives.
• If there are unsaved comments, "Unsaved" appears.
  • Clicking it with nothing selected jumps you to the first unsaved comment.
  • Clicking it with anything selected jumps you to the next unsaved comment after the cursor.
  • Note that unsaved comments can not normally be selected, so this may be weird/buggy.
  • If there are any unsaved comments, the header background is yellow.
• If there are saved but unsubmitted comments, "Unsubmitted" appears.
  • Clicking it with nothing selected jumps you to the first unsubmitted comment.
  • Clicking it with anything selected jumps you to the next unsubmitted comment after the cursor.
  • Note that this is sort of useless (there is normally no reason to cycle through your unsubmitted comments, and the "next step" you'd likely want is to submit them) but consistent with the other buttons.
  • If there are any unsubmitted comments, the header background is yellow.
• If there are any comments, "X / Y Comments" appears.
  • Clicking it with nothing selected jumps you to the first visible comment not marked "Done".
  • Clicking it with anything selected jumps you to the next visible comment not marked "Done".
  • Note that this is sort of useless for non-authors (there is no reason to cycle through comments not marked "done", and they can't do anything about them).
  • If there are no comments which are not marked "Done", this button does nothing.
• The "Next" button is always visible but disabled by default. It becomes enabled when at least one comment exists.
  • Clicking "Next" with nothing selected jumps you to the first visible comment.
  • Clicking "Next" with anything selected jumps you to the next visible comment.
  • Thus: scrolling past some comments, then clicking "Next", may jump you backwards in the document. Your scroll position does not impact the behavior of "Next".
  • "Prev" works like "Next" but in reverse.
• Add settings menu; I'll figure out exactly what's ending up in there separately.
• None of this will be accessible on mobile.
• I will also remove mobile multi-line comments.

Do you want a mobile design? I think we can just hide a div on desktop?

(I'm also ok with none of this on mobile)

I think it's fine for none of this to work on mobile for now.

We might eventually want a way to put some of the "settings" items into the UI on mobile but I don't think it's a priority.

As a user, I don't personally like this UI very much (I don't remember ever forgetting about unsaved/unsubmitted comments and I don't use "Done", so these new elements are just distracting), but I don't see any other support for the "objectives" UI in T12733 and I recall some users asking for a Review Board-style persistent header like this one in the past.

hmmm, I'm happy to take another pass here. For myself I feel like you, I don't need it - if I keep reviews small - but I think that's not something we can apply across all our users. I've also found I don't need "done" until I do. There reaches a point where I want to make sure I've addressed everything before sending it back for review, and then that list I feel is really useful.

I also have some ideas around a more minimal UI if you'd like to see one more pass.

I don't think we need to keep iterating here -- I don't think any version of this will ever be useful to me personally, and I think I'm clearly fighting a losing battle by trying to keep all the "X / Y" progress-bar-style elements out of the UI.

I can also just put next/prev back on inlines if you prefer, and we can deal with the design issue if/when we add a context menu for "View History", "View Raw Remarkup", etc. (T11401).

What about hiding the x/n count until you've marked 1 as done?

I think this feature is just inherently incompatible with my hardline "review shouldn't be a progress bar" view, and it would be more confusing to try to obscure the progress aspect of it.

I'm fine with dropping "no progress bars". I believe most requests we've ever received about inlines -- like T8909#224084 -- are fundamentally misguided. That user wanted features to make managing a review with 100 spelling corrections easier. But these requests are common and I don't have any real reason to believe that making Differential hard to misuse is actually helping anyone or putting any real pressure on attacking the root issues.

Yeah, I tried hard to not make it progress-like. My approach was mostly, "if I have a dozen comments across 5 files, how could I assume I've read/addresses each piece of feedback" - and that still leads to some sort of progress bar. If people don't click "done" I think it's reasonable to assume they don't need that level of organization. I also suffer from ADHD and have a difficult time if I don't maintain things in a list.

Are those buttons rendered by any existing class, or are they novel elements? The circular menus are presumably PHUIIconCircleView but the other buttons look custom (it looks like the spacing is slightly different from the existing "Simple" buttons, maybe)? Should I add a new mode to PHUIButtonView, or is this an existing mode somewhere that I'm just missing, or are they just "Simple" buttons with slightly Photoshoppy rendering?

Just use SIMPLE for now and I'll retouch as needed. It's probably a little over styled in the mock.

Okay. I have to port whatever we're using to PHUIX so you'll have to retouch both PHP and JS, but I can maybe put PHUIX rendering beside PHUI rendering in UIExamples to make that easier.

Yes I slimmed them in photoshop :(

Is there a place in 2-5 years where we can write one UI library and work in PHP and JS

You can just make the circle button a square button as well.

Though I do circle in JS in lightbox, so there would be a second application

And, uh, SIMPLE is a color. I'm going to change that first, unless you have some other plan for having the yellow and non-yellow buttons present in the mock?

Oh there is no PHUIXButtonView...

(I don't have any plans for making JS and PHP render from the same authority -- I don't see any realistic way to accomplish it.)

what is this even

bad zoot

If you're making PHUIXButtonView I can come behind and merge in circle stuff. I don't think that needs to be a separate PHP class. I just need to dig out all the button-css stuff and make it more modular so it's not silly with overrides.

Sounds good. Current trajectory is:

• "Simple" isn't a color.
• PHUIXButtonView