Page MenuHomePhabricator

Revamp inline commenting UI
ClosedPublic

Authored by chad on Mar 26 2015, 4:59 PM.

Details

Summary

Rebuilds the UI in Differential commenting. Specifically we look at the following design patterns:

To the author:

  • The author of the diff should be able to easily identify what comments are done and not done.
  • We keep undone comments yellow
  • Clicking done turns comment block into 'unsubmitted state'

To the reviewer:

  • Easier understanding of unsubmitted states
  • All conversations to be yellow/important

Todo

  • Not all color CSS states correct
  • Unpulished checkbox support
Test Plan

Test leaving comments, published and unpublished. Checking Done, unpublished and published. Check delete states.

From the Diff Author's perspective:

For a Diff commenter's perspective:

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chad updated this revision to Diff 29245.Mar 26 2015, 4:59 PM
chad retitled this revision from to [Draft] Revamp inline commenting UI.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, btrahan.
chad updated this revision to Diff 29255.Mar 26 2015, 6:15 PM
  • Move to tooltips, iconfont, rebase
chad updated this object.Mar 26 2015, 6:18 PM
epriestley edited edge metadata.Mar 26 2015, 6:39 PM

Let me poke around this and I'll either steal it or we can just land it and I'll shoot you some followups, one sec..

The inlines look good but this stuff got caught in the crossfire:

Is that expected / did I patch wrong?

Oh, I think the inline CSS isn't being included anymore if there are no inlines on the page. It was probably packaged previously and no longer is. As soon as you actually submit an inline it fixes itself.

With the styles in, preview is a bit funky:

The "down" arrow, which should be labeled "Next", is labeled "Previous".

When "Done" is disabled, it would be nice not to highlight it on hover maybe?

I don't love the "Attach Draft" text, mostly because of the word "Attach". If you want to change it, how about "Save Draft"?

chad added a comment.Mar 26 2015, 6:48 PM

Oh yeah I split up the css files. Might need another require

chad planned changes to this revision.Mar 26 2015, 7:51 PM

So on the CSS, I feel like its specificity whack-a-mole. At this point, UI wise, we have just two states, default and highlighted in actual use. I might just move the logic to JS/PHP instead of writing lots of specific CSS rules, which should end up being cleaner and easier to understand.

I can make the other changes pretty easily. I assume this is the general correct direction?

Yeah, I think this is great other than the handful of minorly glitchy things I caught.

chad updated this revision to Diff 29283.Mar 27 2015, 5:37 PM
chad edited edge metadata.
  • Move to tooltips, iconfont
  • Checkpoint
  • Proper hover states
chad updated this revision to Diff 29285.Mar 27 2015, 5:38 PM
  • Move to tooltips, iconfont
  • Checkpoint
  • Proper hover states
chad added a comment.Mar 27 2015, 5:39 PM

This isn't ready yet, just checkpointing and rebasing.

chad updated this revision to Diff 29288.Mar 27 2015, 6:46 PM
  • Rebase
  • Synthetic Styles
  • basic mobile styling pass
chad added a comment.Mar 27 2015, 7:00 PM

I'm down to just implementing you're last diff. Plus some JS I think you have a TODO for on the 'done' state (setting is-unsubmitted')

chad added a comment.Mar 27 2015, 7:00 PM

*your

ugh

chad updated this revision to Diff 29289.Mar 27 2015, 8:45 PM
  • Add CSS for viewer-is-diff-author
chad added a comment.Mar 27 2015, 8:46 PM

OK, I think this is reviewable. Only outstanding piece is:

  • JavaScript should set a class to show "Unsubmitted Checkbox" when Done button clicks.

I think you were taking care of that?

chad retitled this revision from [Draft] Revamp inline commenting UI to Revamp inline commenting UI.Mar 27 2015, 8:46 PM
chad edited the test plan for this revision. (Show Details)Mar 27 2015, 8:49 PM
chad added a comment.Mar 27 2015, 8:52 PM

Hmm, maybe just making all "done" comments Grey... looking at it when taking a step back it seems not particularly useful distinction. Might have to see it live.

Sorry, slow day here -- looking through this shortly. And, yeah, that JS thing is on my plate.

chad added a comment.Mar 27 2015, 10:40 PM

Yeah I think I want to change this to always be grey if 'done'. Also, is there a good reason to keep the checkbox if you can't mark as done? I'd think an icon treatment would display better.

If canmarkdone: real checkbox + Done
if !canmarkdone: cool icons: Not Done, Done

epriestley accepted this revision.Mar 27 2015, 10:48 PM
epriestley edited edge metadata.

Using an icon is totally fine, I just used a checkbox out of laziness. There's no interaction which "activates" the control without a page reload (you can Commandeer, but that will refresh everything) so there's no technical reason we need to render a control. Feels like an icon might work better to me, too.

The only thing that strikes me as a touch odd is the hover state on "Done" when the box is yellow. The blue-on-white feels a little incongruous.

The rest of this looks really good to me. Kick this in whenever you're happy with it and we can tweak it a bit in followups?

This revision is now accepted and ready to land.Mar 27 2015, 10:48 PM
chad added a comment.Mar 27 2015, 10:51 PM

Yeah that hover state is a buggypoo

chad updated this revision to Diff 29290.Mar 27 2015, 10:59 PM
chad edited edge metadata.
  • Use "author" if comment is from diff author
  • Grey out any "done" box, viewer not dependent
  • Fix hover state on "done" box
This revision was automatically updated to reflect the committed changes.
chad added a comment.Mar 27 2015, 11:01 PM

I'll follow up with the icons/done revamp later tonight.

Cool. I'll fix the JS state and provide those missing author hints.