Page MenuHomePhabricator

Show file comments on file lightboxes

Authored by chad on Nov 18 2016, 12:00 AM.



Basic work in progress, but should show timeline comments for files when in lightbox mode. Looks reasonable.

Test Plan

click on images, see comments from timeline.

Diff Detail

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

Event Timeline

chad updated this revision to Diff 40673.Nov 18 2016, 12:00 AM
chad retitled this revision from to Show file comments on file lightboxes.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
chad updated this revision to Diff 40674.Nov 18 2016, 12:07 AM
  • less lint issues
chad updated this revision to Diff 40676.Nov 18 2016, 5:21 AM
  • style the timeline
epriestley edited edge metadata.Nov 18 2016, 1:24 PM

Instead of adding the fileID to the metadata, could we just use the existing filePHID right next to it? This isn't a big deal, but it's a little redundant to pass them both around and if we want to do something in the future like let you click a macro or a Pholio thumbnail or Paste or whatever to bring up a lightbox with comments (or just render comments in this way in Phriction, which I think we do want) it will probably be easier if the code is PHID-based so it can work on objects other than files.

I think this is pre-existing, but I'd find it useful if there was a way to get to the /F123 page from the lightbox. Maybe we could link the monogram in the upper left?

The comments don't actually load for me locally in Safari, I just see this:

Maybe a Safari issue? I can dig into it if you can't reproduce.

Stuff you want for v0 is basically this, right?

  • Some way to render these using Conpherence view code.
  • Test for a timeline view being empty.

I ideally want this, too, but can counterdiff you when I have a chance:

  • Convert Files to EditEngine (T11357).
  • Serve this view from the standard EditEngine controller so any object can render this lightbox thread view.

We probably need to implement some kind of $timeline->getIsEmpty() thing.


This should be markCommentsLoading(true) -- comments are now loading?

I think it's slightly better to sequence this code like this:

new Workflow.start();

That is, mark loading first, then start the workflow.

If you do it in the other order, it's possible that we'll introduce some kind of, e.g., caching or optimization into Workflow later (or change JX.Workflow to a different call to fetch data) that makes it return immediately, inside start(). If we do, code like this won't work:

new Workflow()
  .start(); // <-- If we add some kind of cache, this will call set_loading(false); before it returns!

There isn't any particular reason to believe we'll make this change, but I think the code is more robust overall in the other order.

chad added a comment.Nov 18 2016, 2:44 PM

I use ID in the header for F1234 as well. I hadn't thought about using this for Macro/Pholio. I planned to link the Monogram, like you mentioned. Is that possible with just a PHID?

You probably should just generate it on the server anyway and pass monogram or objectName or something anyway so we aren't duplicating the code for building monograms.

chad added a comment.Nov 18 2016, 4:23 PM

For v0 I just need is_empty I think. There is too much Conpherence UI specific stuff (date markers) in it for this. I did try to do all of this in PHUITimelineView, but had issues (500 server errors) when trying to pass in an extra parameter.

After edit-engine stuff... I can wire up actually posting from this form, I think.

I also want all files (not just viewable) to be comment-able, so I'd probably see if that's short and easy too. So if you upload an XLS file, even if we just show you a file icon, people could comment on it.

Subscribe button as well.

So yeah a few follow up diffs. I can repro Safari, but I'm not familiar with it's debugging tools, should be able to fix that though.

chad updated this object.Nov 18 2016, 4:30 PM
chad edited edge metadata.
chad added a task: T3612: Lightbox v2.

I think this ends up being fairly easy:

diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
index 6e27f85..5eaa528 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
@@ -233,6 +233,10 @@ class PhabricatorApplicationTransactionView extends AphrontView {
     return $view;
+  public function isTimelineEmpty() {
+    return !count($this->buildEvents(true));
+  }
   protected function getOrBuildEngine() {
     if (!$this->engine) {
       $field = PhabricatorApplicationTransactionComment::MARKUP_FIELD_COMMENT;

That's pretty inefficient, but should be fine for now to let this move forward.

chad updated this revision to Diff 40678.Nov 18 2016, 5:57 PM
  • switch to sending monogram
  • hyperlink monogram
  • fix safari
  • add empty state
  • move loading triggers (no css yet)
chad added a comment.Nov 18 2016, 8:59 PM

I think I got all of your concerns?

epriestley accepted this revision.Nov 18 2016, 9:08 PM
epriestley edited edge metadata.

Oh, sorry, I thought "no css yet" was like "going to add that before you look at this". Everything looks reasonable to me.


This is still a bit funky but it doesn't really matter until there's CSS.


(Weird indent.)

This revision is now accepted and ready to land.Nov 18 2016, 9:08 PM
chad added a comment.Nov 18 2016, 9:10 PM

I didn't add loading CSS but everything else is there.

chad updated this revision to Diff 40683.Nov 18 2016, 9:21 PM
chad edited edge metadata.
  • add very basic animation, not flashy, very boring
This revision was automatically updated to reflect the committed changes.