Conpherence - make the durable column kind of work and stuff
ClosedPublic
Actions

Authored by btrahan on Mar 4 2015, 8:34 PM.

Details

Reviewers

epriestley
chad

Maniphest Tasks

T7014: Annouce Persistent Chat option in Conpherence

Commits

Restricted Diffusion Commit
rP27a60bdb3c8b: Conpherence - make the durable column kind of work and stuff

Summary

Ref T7014. This hooks up the durable column such that when you open it up it loads your most recent Conpherence. You can then switch amongst the various widgets and stuff and everything works nicely.

Except...

scroll bar does not work
- also doesn't work at HEAD when I add a ton of text to the UI with no changes? (wrapped $copy in array_fill(0, 1000, $copy))
"widget selector" does not collapse when you select something else
- this part wasn't really specified so I used the aphlict dropdown stuff. didn't want to keep working on that if this was the wrong UI choice
can not edit title
- do we still want that to be done by clicking on the title, which pops a dialogue?
can not add participants or calendar events
- what should this UI be? maybe just a button on the top for "participants" and a button on the bottom for calendar? both on top?
this is not pixel perfect to the mock or two I've seen around. Aside from generally being bad at that, I definitely didn't get the name + timestamps formatting correctly, because the standard DOM of that has timestamp FIRST which appears second due to a "float right". Seemed like a lot of special-casing for what might not even be that important in the UI so I punted. (And again, there's likely many unknown ways in which this isn't pixel perfect)

There's also code quality issues

ConpherenceWidgetConfigConstants is hopefully temporary or at least gets more sleek as we keep progressing here
copied some CSS from main Conpherence app
- DOM structure is pretty different
- there's some minor CSS tweaks too given the different width (not to mention the DOM structure being different)
copied some JS from behavior-pontificate.js to sync threads relative to aphlict updates
JS in general is like a better version of existing JS; these should collapse I'd hope?
maybe the aphlict-behavior-dropdown change was badsauce?

...but all that said, this definitely feels really nice and I feel like adding stuff is going to be really easy compared to how normal Conpherence is.

Also includes a bonus bug fix - we now correctly update participation. The user would encounter this issue if they were in a conpherence that got some updates and then they went to a different page; they would have unread status for the messages that were ajax'd in. This patch fixes that by making sure we mark participation up to date with the proper transaction in all cases.

Test Plan

hit "\" to invoke the column and saw nice loading UI and my latest conpherence load. sent messages and verified they received A-OK by looking in DOM console. toggled various widges and verified they rendered correctly. opened up a second browser with a second user on the thread, sent a message, and it was received in a nice asynchronous fashion

Diff Detail

Repository

rP Phabricator

Branch

T7014

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/conpherence/controller/ConpherenceColumnViewController.php:11	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 4746
Build 4760: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

btrahan updated this revision to Diff 28816.Mar 4 2015, 8:34 PM

btrahan added a task: T7014: Annouce Persistent Chat option in Conpherence.

btrahan retitled this revision from to Conpherence - make the durable column kind of work and stuff.

btrahan updated this object.

btrahan edited the test plan for this revision. (Show Details)

btrahan added a reviewer: epriestley.

Herald added a reviewer: chad. · View Herald TranscriptMar 4 2015, 8:34 PM

Herald added subscribers: epriestley, Korvin. · View Herald Transcript

btrahan updated this object.Mar 4 2015, 8:45 PM

btrahan edited edge metadata.

do we still want that to be done by clicking on the title, which pops a dialogue?

I feel like this behavior is a little confusing, but we should probably stick with it unless we're changing Conpherence too.

what should this UI be? maybe just a button on the top for "participants" and a button on the bottom for calendar? both on top?

Not sure, although we should probably try to unify the full-app and column-only UIs.

Overall, this generally seems reasonable and we have a lot of latitude to refine it while it's hidden behind \.

src/applications/conpherence/controller/ConpherenceColumnViewController.php
10	Maybe just infer "shouldInit" from lack of "id"? Looks like there are only two real options here.
51	Re-rendering the whole column feels maybe-bad-ish (e.g., we'll lose text the user has started typing, I think? Haven't gotten to the JS) but we can refine that later.
src/applications/conpherence/view/ConpherenceDurableColumnView.php
40–41	Consider setting these in `getTagAttributes()` if they aren't here for a reason.
src/view/page/PhabricatorStandardPageView.php
86–87	Weird that this returns a user instead of a bool.
webroot/rsrc/js/application/conpherence/behavior-durable-column.js
121	Scrollbar issue is probably that this needs to be invoked on something again after loadThreadContent() happens.

This revision is now accepted and ready to land.Mar 4 2015, 10:31 PM

Thanks for the feedback! I need a little more help on two of the inlines when you have a chance.

src/applications/conpherence/controller/ConpherenceColumnViewController.php
10	I was thinking that if users visit this end point they should generally get a 404. I guess I should just an isAjaxRequest check instead?
src/view/page/PhabricatorStandardPageView.php
86–87	Haha, whoops. I fixed this to be if the conpherence application is installed for the user
webroot/rsrc/js/application/conpherence/behavior-durable-column.js
121	I did some debugging and it seems JX.Scrollbar._getScrollbarControlWidth() returns 0 as called here? (I debugged by making this private call right after this init call) _getScrollbarControlWidth: function() { var self = JX.Scrollbar; if (self._controlWidth === null) { var tmp = JX.$N('div', {className: 'jx-scrollbar-test'}, '-'); document.body.appendChild(tmp); var d1 = JX.Vector.getDim(tmp); tmp.style.overflowY = 'scroll'; var d2 = JX.Vector.getDim(tmp); JX.DOM.remove(tmp); self._controlWidth = (d2.x - d1.x); } return self._controlWidth; } I'm surprised `frame` isn't involved in this logic instead of `document`. I also don't know enough about overflow to say with certain if there is some sort of bug in it or not.

I don't think there's any real reason not to just give them JSON if they manually edit the URL to point at some weird place. It's helpful for debugging, and it should be unsurprising that typing /internals/secrets/raw/hack/ into your browser doesn't produce a usable page.

I probably have to play with the Scrollbar stuff locally to be of any help there -- kick it in and I'll see if I can figure it out?