Page MenuHomePhabricator

Polish up timeline for PHIUTwoColumnView
ClosedPublic

Authored by chad on Mar 8 2016, 10:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Sep 8, 11:34 PM
Unknown Object (File)
Sat, Aug 31, 5:18 AM
Unknown Object (File)
Fri, Aug 30, 9:23 AM
Unknown Object (File)
Sun, Aug 25, 9:19 PM
Unknown Object (File)
Sun, Aug 25, 3:24 PM
Unknown Object (File)
Sat, Aug 24, 11:31 PM
Unknown Object (File)
Thu, Aug 22, 3:56 AM
Unknown Object (File)
Aug 17 2024, 9:21 PM
Subscribers

Details

Reviewers
epriestley
Commits
Restricted Diffusion Commit
rP3d44a5c253ad: Polish up timeline for PHIUTwoColumnView
Summary

This inverts colors and icons a bit, so they're not as harsh. So instead of a dark green item with white icon, its now light green with a dark green icon. I've also changed all text and comment boxes to be "grey" visually to separate out the UI from converation/actions. Give it a spin and let me know how this feels. I still need to update the comment UI.

Test Plan

UIExamples, lots of various tasks and diffs.

pasted_file (721×467 px, 79 KB)

pasted_file (802×268 px, 80 KB)

Diff Detail

Repository
rP Phabricator
Branch
timeline-css (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 11085
Build 13724: Run Core Tests
Build 13723: arc lint + arc unit

Event Timeline

chad retitled this revision from to Polish up timeline for PHIUTwoColumnView.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.

Ideally, I'd like to separate out the events that sit in the comment header like the bottom of this screen:

pasted_file (802×268 px, 80 KB)

Like CurtainView, I think we can provide richer UI stories on certain actions (like a bad build, closed task, archival, etc), but they won't work inside the comment area header.

epriestley edited edge metadata.

The default icon color is maybe a little too dark? I think the current scheme sort of puts transactions at extremes of either nearly-hidden or extremely-visible (e.g., "Accept" and "Reject" are very bold and colorful, while normal edits are nearly invisible).

This makes the currently-hidden stuff much more visible, and the super-bold stuff less visible, so everything kind of meets in the middle, but I think most of the unimportant events aren't interesting when scanning the timeline so having them be more visible isn't necessarily important? So my initial gut is to maybe tone the contrast on the foreground default color down slightly little so unimportant events don't get as much of a visibility bump.

I think the in-header icons lost a lot of visibility too when losing the background color ("accept" and "reject" are particular cases), but maybe that's moot.

Happy to see if I still feel this way after using it for a week or two, though.

I think we can provide richer UI stories on certain actions (like a bad build, closed task, archival, etc), but they won't work inside the comment area header.

We can always force a particular story type out of the header if you want to enrich it (this is easy), so we can do this without separating everything if you want.

This revision is now accepted and ready to land.Mar 8 2016, 10:11 PM

(I do like this overall and I think it's an aesthetic improvement, I'd just maaaaaaaaaaaaybe like slightly smaller changes to relative visibility of events.)

I go back and forth on it, (dark colors vs. light colors). Let me ponder it a bit.

I'm OK with removing all even groupings from inside the comment area, the richness is just an extra bonus.

I'm OK with removing all even groupings from inside the comment area, the richness is just an extra bonus.

This is easy (basically, just delete tons of code), but I think the grouping is pretty useful in, e.g., Maniphest, to show when comments relate to a bunch of actions, when a subscribe is a consequence of a mention, etc. Do you think this stuff isn't useful?

Basically, this diff:

diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
index 8b9955a..72a0548 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
@@ -368,7 +368,7 @@ class PhabricatorApplicationTransactionView extends AphrontView {
     $groups = array();
     $group = array();
     foreach ($xactions as $xaction) {
-      if ($xaction->shouldDisplayGroupWith($group)) {
+      if ($xaction->shouldDisplayGroupWith($group) && false) {
         $group[] = $xaction;
       } else {
         if ($group) {

...will turn this:

Screen Shot 2016-03-08 at 2.20.26 PM.png (191×1 px, 43 KB)

...into this:

Screen Shot 2016-03-08 at 2.20.36 PM.png (292×1 px, 68 KB)

...but that seems like a pretty big step backward to me.

Oh sorry, I still want them grouped, I just want the comment box first, then I can "attach" the actions below. Still one group. Basically I think it'll read a with a little more empathy. Something like:

  • Knock Knock
  • Hi we're selling girl scout cookies.
  • Close Door
  • I bought some last week, thanks.

vs.

  • Knock Knock
  • Hi we're selling girl scout cookies.
  • I bought some last week, thanks.
  • Close Door

Ohhhhhhh, okay.

Stuff like "add cc" and "reassign" (and maybe some other stuff like commandeer and resign) reads perhaps a little more "naturally" to me on top, but I agree that accept/reject/close task maybe make more sense on the bottom.

We could do it per-transaction, but maybe that would be weird to have some stuff on top and some stuff underneath.

i.e., we get this if we put everything on the bottom, including subscribe/reassign:

  • I'll take 5 boxes of cookies.
  • Hey, Girls Scouts!

Less of an empathy problem but not completely natural.

Yeah in general I like when we shorten the action into the header, for single action cases of low importance. It might be weird though too...

I planned to just play with something in my sandbox and see if something felt right.

Maybe it is just a case of "special transactions" that must not be included in the header of the comment. So we mark opening and closing of a task as special, and it always renders independent?

So we mark opening and closing of a task as special, and it always renders independent?

Yeah, that's pretty easy to do and seems reasonable to me (although getting the ordering right may be a little tricky, I'll have to look at the code). The ones that jump out at me are:

  • Task status changes (maybe only closes?).
  • Accept/reject in Differential (maybe only reject?).

I can't think of any other cases where there's a possible empathy/chronology issue offhand, do you have any in mind?

I guess accept/reject (or only reject?) in Diffusion too.

Any that we want to stylize. We're just back to that. So if we make a "Added Projects" that actually renders the tag, show it underneath the comment.

That feels like the shortest path here, group them in the comment header unless they have special UI, then attach them underneath? Pholio could actually render mocks, etc.

It seems a little weird to me to require enriched content to go below the comment -- for example, if we do render mocks inline, I probably want people to look at the mock before I discuss it? Won't my comment make no sense if they haven't seen the mock yet? Or I'll have to start with "here's a mock (at the end of my comment, scroll down and look at it first, then come back here)..."?

Can we just find some other way to put richer content in the comment group? What's your concern with, e.g., just dumping the content into the existing header as it stands, or adjusting the design treatment there?

Changing the ordering makes sense to me from an empathy/chronology viewpoint for "closed this task as invalid", but it doesn't make sense to me to, say, move subscribers down just because we want to render user tags or something.

Maybe we could render the grouping differently instead? For example, just put all the actions closer together with a single profile image to show the group? We already sort of do this:

Screen Shot 2016-03-08 at 3.05.56 PM.png (456×812 px, 99 KB)

...but we could do that for comments too, and make the picture big if the group had a comment in it, then not actually put all the related transactions into the panel itself.

(And maybe, say, combine that with a little more space between groups to emphasize the borders better, since the boundaries will be a little less distinct than they currently are.)

It seems a little weird to me to require enriched content to go below the comment -- for example, if we do render mocks inline, I probably want people to look at the mock before I discuss it? Won't my comment make no sense if they haven't seen the mock yet? Or I'll have to start with "here's a mock (at the end of my comment, scroll down and look at it first, then come back here)..."?

I don't think it's odd, really. That is, the comment -> attachment/actions paradigm is common enough (Facebook, Twitter) that I don't think anyone would find it strange. We do this with Differential/Audit anyways. Mostly of the time all I really care about reading a Diff or Task is reading the actual text another human wrote. Timeline stuff isn't generally stuff I seek out.

We can give it a shot. I think it's going to be pretty weird (we don't fold/limit comments, while Facebook and Twitter do), but maybe not.

I'm still worried we're not talking about the same thing, hah. We can come back to this later, will think about it some more.

D15443 is what I think you're talking about, but maybe that's not what you're actually talking about.

It is, thanks, I'll pull it down and play around with it locally. Let me clean this diff up first and see where we're at. I did tighten up spacing on grouped actions, etc.

chad edited edge metadata.
  • bolder colors.
This revision was automatically updated to reflect the committed changes.