Page MenuHomePhabricator

Workboards - remove 7 column restriction
ClosedPublic

Authored by btrahan on May 2 2014, 12:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 4:22 AM
Unknown Object (File)
Thu, Nov 21, 1:26 AM
Unknown Object (File)
Wed, Nov 20, 5:43 PM
Unknown Object (File)
Wed, Nov 20, 5:43 PM
Unknown Object (File)
Wed, Nov 20, 5:43 PM
Unknown Object (File)
Sun, Nov 17, 9:14 AM
Unknown Object (File)
Sun, Nov 10, 10:06 PM
Unknown Object (File)
Tue, Nov 5, 2:05 AM

Details

Summary

Fixes T4914. We currently have a finite limit on column displays which caused T4914. This fixes T4914 by no longer using a fluid layout. Rather, we use a fixed column width layout which does not have a 7 column limit. Future work - see T4054 for an example - will likely make the fluid layout thing work with infinite columns, and / or other work may re-jigger project workboards directly.

Test Plan

had a project like in T4914 that wouldn't load and it loaded post this change! added more columns and using javascript inspector noted proper width being set

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

btrahan retitled this revision from to Workboards - remove 7 column restriction.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added reviewers: epriestley, chad.

I would at least bump this up to 10 for now and kick the can down the road.

webroot/rsrc/css/aphront/multi-column.css
64

The big concession as I understand it - if you have some glorious 3200pixel display with a hojillion columns, you will see 1600 pixel wide that then snaps to 3200+ based on how many a hojillion really is. I am using a 15" MacbookPro with Retina display though and this is already hugely big.

webroot/rsrc/js/core/behavior-multi-column.js
18 ↗(On Diff #21213)

My idea was that this works for all amounts of columns, kicking the old can infinitely far down the road as far as functionality is concerned.

Not sure if the 200 pixel per column math really holds true, and I suspect you'd like to have widths autoscale based on the viewport and whatnot.

Can we just make these columns always be 320px-ish wide? The 1 and 2 column cases are fairly silly looking at 100% and 50% width, and things get better at 3-5 columns and then the columns get unusably tiny and finally explode at 7. Then we don't need any JS or magic.

As a point of comparison, Trello columns are always 260px-ish wide.

Multicolumnview supports both fixed and fluid.

Any objection to switching to fixed? Did I just write this weird originally by picking fluid?

Yeah I just like fluid, you get more value out of the real estate. Is there a way to do both? Min-width something? I'd have to look at the CSS again

Here's the hacky fixed-width version -- major advantage is no JS stuff.

diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php
index a697619..2cf8b8e 100644
--- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php
+++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php
@@ -89,7 +89,6 @@ final class PhabricatorProjectBoardViewController
 
     $board = id(new PHUIWorkboardView())
       ->setUser($viewer)
-      ->setFluidishLayout(true)
       ->setID($board_id);
 
     $this->initBehavior(
diff --git a/src/view/layout/AphrontMultiColumnView.php b/src/view/layout/AphrontMultiColumnView.php
index e314cb2..a12a4f1 100644
--- a/src/view/layout/AphrontMultiColumnView.php
+++ b/src/view/layout/AphrontMultiColumnView.php
@@ -46,7 +46,9 @@ final class AphrontMultiColumnView extends AphrontView {
     $classes[] = 'grouped';
 
     if (count($this->columns) > 7) {
-      throw new Exception("No more than 7 columns per view.");
+      if ($this->fluidLayout || $this->fluidishLayout) {
+        throw new Exception("No more than 7 columns per view.");
+      }
     }
 
     $classes[] = 'aphront-multi-column-'.count($this->columns).'-up';
diff --git a/src/view/phui/PHUIWorkboardView.php b/src/view/phui/PHUIWorkboardView.php
index 06ee373..835a94e 100644
--- a/src/view/phui/PHUIWorkboardView.php
+++ b/src/view/phui/PHUIWorkboardView.php
@@ -3,8 +3,6 @@
 final class PHUIWorkboardView extends AphrontTagView {
 
   private $panels = array();
-  private $fluidLayout = false;
-  private $fluidishLayout = false;
   private $actions = array();
 
   public function addPanel(PHUIWorkpanelView $panel) {
@@ -12,16 +10,6 @@ final class PHUIWorkboardView extends AphrontTagView {
     return $this;
   }
 
-  public function setFluidLayout($layout) {
-    $this->fluidLayout = $layout;
-    return $this;
-  }
-
-  public function setFluidishLayout($layout) {
-    $this->fluidishLayout = $layout;
-    return $this;
-  }
-
   public function addAction(PHUIIconView $action) {
     $this->actions[] = $action;
     return $this;
@@ -57,12 +45,6 @@ final class PHUIWorkboardView extends AphrontTagView {
 
     $view = new AphrontMultiColumnView();
     $view->setGutter(AphrontMultiColumnView::GUTTER_MEDIUM);
-    if ($this->fluidLayout) {
-      $view->setFluidLayout($this->fluidLayout);
-    }
-    if ($this->fluidishLayout) {
-      $view->setFluidishLayout($this->fluidishLayout);
-    }
     foreach ($this->panels as $panel) {
       $view->addColumn($panel);
     }
diff --git a/webroot/rsrc/css/aphront/multi-column.css b/webroot/rsrc/css/aphront/multi-column.css
index 6fe0e6f..2005487 100644
--- a/webroot/rsrc/css/aphront/multi-column.css
+++ b/webroot/rsrc/css/aphront/multi-column.css
@@ -28,7 +28,7 @@
 }
 
 .aphront-multi-column-fixed .aphront-multi-column-column-outer {
-  width: 300px;
+  width: 280px;
 }
 
 /* flexible, but with a minimum */
diff --git a/webroot/rsrc/css/phui/phui-workpanel-view.css b/webroot/rsrc/css/phui/phui-workpanel-view.css
index f1ef1bc..24b4ab2 100644
--- a/webroot/rsrc/css/phui/phui-workpanel-view.css
+++ b/webroot/rsrc/css/phui/phui-workpanel-view.css
@@ -53,7 +53,7 @@
 }
 
 .aphront-multi-column-fixed .phui-workpanel-body {
-  width: 300px;
+  width: 280px;
 }
 
 .phui-workpanel-body .phui-object-item-list-view {

Specific issues I see with this JS approach are:

  • JS is sorta yuck
  • I think these columns get unreasonably narrow, particularly with the current card approach:

Screen_Shot_2014-05-01_at_7.04.59_PM.png (1×1 px, 155 KB)

  • At least in Safari, we often end up with a big unused gutter of unused space on the right for me.
  • The full-width version with 1 or 2 columns is kind of silly when it's fluid, maybe? Particularly, there's no UI affordance that you can add more columns, since the entire UI is full. In Trello, it's obvious that you can add more columns (Trello also has a specific "Add a column.." affordance).

Screen_Shot_2014-05-01_at_7.06.36_PM.png (929×1 px, 129 KB)

The silliness is mostly only in the nux though. The point of boards being to make boards. A phantom "add column" could improve new board experience and go away after a panel or two is made.

Or we're overthinking this

I do like the idea of improving the new board states though

epriestley edited edge metadata.

Yeah, agreed on overthinking. Whatever its faults, this is clearly better than exploding on 8+ columns, and we have like 10 things in queue to improve boards which will give us plenty of opportunities to fix this properly and do tweaks like empty-board-states.

This revision is now accepted and ready to land.May 2 2014, 2:21 AM

I'm just wondering when this diff will be landed? We are going to upgrade our Phabricator install once this happens.

btrahan edited edge metadata.

fixed width version. we basically just

  • stop setting this thing to fluid at the controller level
    • NOTE otherwise fluid functionality still as it was before pre patch
  • only through 7 column exception on fluid layouts (so not for project workboards)
  • tweak fixed CSS width ever so slightly from 300 => 280
btrahan updated this revision to Diff 21383.

Closed by commit rP7f13e8a5c576 (authored by @btrahan).

Why did we change CSS width? Does it look right on mobile now?

Screen_Shot_2014-05-07_at_10.12.53_AM.png (1×828 px, 387 KB)

...is that right?

As to why I made this change, that was my best understanding of what you all wanted me to do based on our discussion yesterday / the diff comments. I would guess the deeper why there is since columns wont get auto-squished in this fixed layout we probably want to be as wide as minimally possible. That said, I admit I am just trying to follow orders. :D

I didn't recall if that's make mobile funky or not. we're good

We should fix the height as well to the browser height so you can see the scroll bar. In this board if you shrink the window, it just terminates the object items and you can't see the scroll bar.

I recall at this point I removed the 'well' around the board columns for simplicity, but it was never used in Workboards, so it never seemed weird. We'll have to bring a well back and absolutely position the height so the scrollbar is always visible.

https://secure.phabricator.com/project/board/483/

@chad - is that something you want me to do or are you doing? If the former, see below as I am confused and generally please do call me out directly if there's something you want me to do. :D

(Fix the height of what to the browser height? By "browser height" do you mean the viewport (the size of the viewable area to the user), the height of the entire rendered document, or maybe something else? Also, whatever that answer is, how do you get that in CSS? (I know in JS... :D)

Do you want to bring the well back stylistically, or perhaps its the thing from sentence 1 that we need to fix the height of to the "browser height", and is just a technique to achieve that end?)

I’m working on a fix. This is my fault, I changed this view but never tested it anywhere other than UIExamples. It needs some work to be ready for abused boards.