Page MenuHomePhabricator

Revamp inline commenting UI
ClosedPublic

Authored by chad on Mar 26 2015, 4:59 PM.
Tags
None
Referenced Files
F12827084: D12171.diff
Thu, Mar 28, 9:47 AM
Unknown Object (File)
Tue, Mar 19, 9:13 PM
Unknown Object (File)
Tue, Mar 19, 8:37 PM
Unknown Object (File)
Tue, Mar 5, 3:10 AM
Unknown Object (File)
Sun, Mar 3, 12:25 PM
Unknown Object (File)
Sun, Mar 3, 11:51 AM
Unknown Object (File)
Feb 14 2024, 1:45 AM
Unknown Object (File)
Feb 5 2024, 7:00 PM
Subscribers
Tokens
"Piece of Eight" token, awarded by btrahan."Manufacturing Defect?" token, awarded by epriestley.

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:

pasted_file (424×667 px, 50 KB)

For a Diff commenter's perspective:

pasted_file (420×652 px, 49 KB)

Diff Detail

Repository
rP Phabricator
Branch
new-inline-comment-differential-ui-its-done-boss
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 5013
Build 5031: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

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.
  • Move to tooltips, iconfont, rebase

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:

Screen_Shot_2015-03-26_at_11.39.36_AM.png (159×561 px, 15 KB)

Screen_Shot_2015-03-26_at_11.40.28_AM.png (278×842 px, 21 KB)

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:

Screen_Shot_2015-03-26_at_11.42.39_AM.png (152×734 px, 16 KB)

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"?

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 edited edge metadata.
  • Move to tooltips, iconfont
  • Checkpoint
  • Proper hover states
  • Move to tooltips, iconfont
  • Checkpoint
  • Proper hover states

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

  • Rebase
  • Synthetic Styles
  • basic mobile styling pass

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')

  • Add CSS for viewer-is-diff-author

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

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.

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 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

Yeah that hover state is a buggypoo

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.

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

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