Page MenuHomePhabricator

Clicking "Done" should set a CSS class on the inline-comment-box
Closed, ResolvedPublic

Description

Can u do dis 4 me. Pls. :3

Main reasoning here is "done" comments should be more of a greyed out "complete" look to them, and un-done should still be yellow so it's easy to spot things you haven't addressed.

Event Timeline

chad created this task.Mar 25 2015, 5:29 PM
chad assigned this task to epriestley.
chad raised the priority of this task from to Needs Triage.
chad updated the task description. (Show Details)
chad added a project: Differential.
chad added a subscriber: chad.

Yeah -- we actually have four states:

  • [ ] Not done
  • [X] You clicked done, but it's an "unpublished" done, so other users can't see it yet.
  • [X] Done
  • [ ] You clicked undone, but it's an "unpublished" undone, so other users can't see it yet.

My thinking was maybe to sync the overall color with whether the checkbox is checked or not, and then show some local style on the checkbox to hint that it's unpublished. For example, crudely:

  • If the checkbox is checked, comment is "complete" looking.
  • If the checkbox is unchecked, comment is "incomplete" looking.
  • If the state of the checkbox hasn't been published, checkbox has an indigo or red or blue or yellow or whatever border around it or something like that.

I also suspect we want replies to count as giving inlines a "complete" look.

Philosophically, I also want to be careful about making these states too bold, since I don't want users to feel like they're obligated to go check things off. Making the states really extreme could make checking boxes feel more mandatory.

Anyway, I'll give you some CSS classes and we can go from there.

chad added a comment.Mar 25 2015, 5:37 PM

I have this little guy for 'unsubmitted', easy to show and hide with CSS rules. Might have to dial it up, but we'll see.

Oh, the actual checkmark itself can be unsubmitted though. Presumably that should be communicated differently than the comment as a whole being a draft.

That is, if you make an inline comment and I check it off as done, no one but me can see that checkmark until I comment on the revision, update it with a new diff, or take another major action. All of the stuff I checked off publishes all at once at that time, as a single "epriestley marked 13 inlines done." event. Until then, when you and I look at the same inline, you'll see it unchecked and I'll see it checked.

chad added a comment.Mar 25 2015, 6:15 PM

Is there a good reason to have each inline-comment in a new <tr> vs. just stacking them in the same cell?

Mostly, it makes the code simpler because it doesn't have to check whether it needs to add a new row or not when adding comments, and doesn't have to check whether it needs to remove a row or not when removing comments.

It also makes layout simpler when there are inlines on both the left and right side of a diff.

chad added a comment.Mar 26 2015, 3:30 PM

I'm going to get this to 95% or so and let you commandeer it for the JS edge cases we're not getting currently.

Cool, works for me.

chad added a comment.EditedMar 26 2015, 3:40 PM

Right now I think its:

  • Unpublished is Grey with Red "Unpublished Draft"
  • Unpublished "Done" is should be the same, but is broken in JS.
  • Published with "Not Done" is Yellow (needs attention)
  • Published with "Done" is Grey (has been attended to)

I would like to see an avatar in there maybe just if comment is from the author. I want to denote author in some regards, but not sure if it should be text or just a slight visual cue.

Unpublished "Done" is should be the same, but is broken in JS.

This state should not be possible: you can not click "Done" on an unpublished draft comment. Am I misunderstanding?

chad added a comment.Mar 26 2015, 4:27 PM

Sorry, that clicking "Done" on a Published comment should grey out the comment box, and display the red "Unpublished Draft" tag

So "Unpublished Draft" means both:

  • This comment itself is not published.
  • This comment is published, but the state of the checkbox is not published.

That seems really confusing to me, especially because I don't think of the checkmark as a "draft". Is there a specific reason you don't want to use a different indicator for "unpublished comment" vs "unpublished checkmark"?

chad added a comment.Mar 26 2015, 4:34 PM

The indicator is just a red bubble with text. If you want to change the text in the bubble to be more concise, thats fine with me. It hasn't felt weird in my sandbox.

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

Should I just toss up what I have? I'm just nitpicking CSS at this point

Yeah, that looks great to me. Specifics of the done/draft state aren't a big deal.

chad closed this task as Resolved.Mar 28 2015, 4:15 PM