Stop using JX.Scrollbar for main page content
ClosedPublic
Actions

Authored by epriestley on May 10 2015, 5:13 PM.

Details

Reviewers

btrahan
chad

Maniphest Tasks

T8151: Decide the fate of JX.Scrollbar

Commits

Restricted Diffusion Commit
rPc94bd8e4f2f8: Stop using JX.Scrollbar for main page content

Summary

Ref T8151. This is option (5). It needs a few adjustments but feels pretty good. Major issues are:

Without a mouse, the scrollbars overlap by default, so we must move the column off the right margin.
Scrolling sometimes "bleeds" between the chat vs the main frame in a way that's not as discrete as the old framed content, but feels generally reasonable to me.

If we pursue this, I'd plan to make these additional changes:

Move the panel away from the right margin only if the page scrollbars are zero-width (i.e., in OSX trackpad mode).
Fix the notch in the upper right corner when the chat is moved away from the right margin.
Probably remove the body "overflow-y: scroll" on Conpherence and Workboards.
Update the resizing code to deal with 300px vs 315px widths.
We can probably clean up some JX.Scrollbar "main panel" code.

Here's the "bad" case, where I've visually separated the column to provide room for a scrollbar. This isn't ideal, but looks and feels OK to me:

Test Plan

Tried Firefox, Chrome, Safari, with and without a mouse.
Tried normal Conpherence.

Diff Detail

Repository

rP Phabricator

Branch

native

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 5888
Build 5908: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley updated this revision to Diff 30742.May 10 2015, 5:13 PM

epriestley retitled this revision from to Stop using JX.Scrollbar for main page content.

epriestley updated this object.

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

epriestley added reviewers: btrahan, chad.

epriestley added a task: T8151: Decide the fate of JX.Scrollbar.

Herald added a subscriber: epriestley. · View Herald TranscriptMay 10 2015, 5:13 PM

epriestley mentioned this in T8151: Decide the fate of JX.Scrollbar.May 10 2015, 5:16 PM

This is what it looks like on windows + chrome:

Also,

plustwo

Snug up column with a mouse connected.
Fix notch.
Update resizing code.

There's a bit more tweaking available, but I think this version is shippable if the double-scrollbar compromises are generally agreed to be the best tradeoff here.

I'm supportive but sad about it; see T8151#113322.

In terms of landing, perhaps we should wait until @chad has a chance to weigh in, and / or add some design feedback for this approach?

webroot/rsrc/js/application/conpherence/behavior-durable-column.js
77 ↗	(On Diff #30753)	whats "!!" do? not sure if typo or some clever casting of "0" to false and "some integer" to true or perhaps some other JS magic.

This revision is now accepted and ready to land.May 11 2015, 6:44 PM

!! is a vaguely-idiomatic (bool) cast in JS, since JS doesn't have casts. The first ! converts it to a boolean (but with the opposite value) and the other one converts it back to the original value.

You're welcome to land whatever people like best. I'll gripe if anything seems untenable.

Sounds good. We can revisit framing content again later, too (say, after Chrome gets a fix for T7740, hopefully). Or this might have a bunch of problems I haven't anticipated and push us back up the list. I'll push secure in a sec.