Differential D12171
Revamp inline commenting UI Authored by chad on Mar 26 2015, 4:59 PM. Tags None Referenced Files
Subscribers Tokens
Details
Rebuilds the UI in Differential commenting. Specifically we look at the following design patterns: To the author:
To the reviewer:
Todo
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
Event TimelineComment Actions 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.. Comment Actions The inlines look good but this stuff got caught in the crossfire:   Is that expected / did I patch wrong? Comment Actions 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. Comment Actions 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"? Comment Actions 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? Comment Actions Yeah, I think this is great other than the handful of minorly glitchy things I caught. Comment Actions 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') Comment Actions OK, I think this is reviewable. Only outstanding piece is:
I think you were taking care of that? Comment Actions 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. Comment Actions Sorry, slow day here -- looking through this shortly. And, yeah, that JS thing is on my plate. Comment Actions 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 Comment Actions 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? Comment Actions
|