Let feed panels render something meaningful-ish
ClosedPublic
Actions

Authored by epriestley on May 8 2014, 3:27 PM.

Details

Reviewers

btrahan

Maniphest Tasks

T4986: Implement new Dashboard panel types

Commits

Restricted Diffusion Commit
rPdadd9a9dd98d: Let feed panels render something meaningful-ish

Summary

Ref T4986. We need to introduce alternate views to make this more pleasant, but let rendering move to engines so it can be shared between panels and controllers.

I also moved some of the pagination logic in to avoid duplicating that.

So far, only Feed works. I'm going to do these gradually since we have ~40-50 of them.

Test Plan

Used global search to check for collateral damage.
Used not-global search too.
Used normal feed.

Screen_Shot_2014-05-08_at_8.19.52_AM.png (834×681 px, 87 KB)

Diff Detail

Repository

rP Phabricator

Branch

dash2

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/search/controller/PhabricatorApplicationSearchController.php:216	XHP16	TODO Comment
Advice	src/applications/search/engine/PhabricatorApplicationSearchEngine.php:533	XHP16	TODO Comment

Unit

No Test Coverage

Build Status

Buildable 299
Build 299: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley updated this revision to Diff 21397.May 8 2014, 3:27 PM

epriestley retitled this revision from to Let feed panels render something meaningful-ish.

epriestley updated this object.

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

epriestley added a reviewer: btrahan.

epriestley added a task: T4986: Implement new Dashboard panel types.

Herald added a subscriber: epriestley. · View Herald TranscriptMay 8 2014, 3:27 PM

epriestley edited the test plan for this revision. (Show Details)May 8 2014, 3:27 PM

Harbormaster completed remote builds in B299: Diff 21397.May 8 2014, 3:28 PM

This is awesome. :D

radsauce

src/applications/feed/query/PhabricatorFeedSearchEngine.php
127–137	What were you thinking for customizing display? A 3rd parameter to this method or maybe some configuration that gets done to the engine object itself? (something like "setRenderMode(PhabricatorSearchEngine::TYPE_PANEL))") ...the answer is maybe in my diff queue.
src/applications/search/engine/PhabricatorApplicationSearchEngine.php
535	i thought we had some PHP_INT_MAX thing, though maybe that's from another company I used to work at?

This revision is now accepted and ready to land.May 8 2014, 6:14 PM

What were you thinking for customizing display?

I'm not 100% sure exactly what it will look like, but I think it will mostly go on the Engine, and we'll have some methods like:

getAvailableStyles[ForList|ForPanel]?()
getStyle()
setStyle()
getNiceStyleMapForHumansToRead()

So, yeah, probably $this->getStyle() on the Engine. I'm going to convert all the Engines first though, so they work properly and are merely ugly to varying degrees.

src/applications/search/engine/PhabricatorApplicationSearchEngine.php
535	It's a constant, but internally we do stuff like: 'LIMIT %d', $limit + 1 ..to see if there's another page or not. If we've set `$limit` to PHP_INT_MAX, that will overflow, which may or may not be OK. Basically, this is safe and works everywhere despite being very dumb.

I think most things in the beginning will only have one render view. Things like feed, ponder, phame, notifications -> feed display. I can't think of 'multiple feed displays' but it may come up.

ObjLists I think I should spend some time building "Normal" and "Compact" views perhaps.

I thought it would be nice to have for a "text" mode at least.

...it would also maybe be advantageous to allow people to extend / configure how these things are rendered? I see requests to add certain data to places that strikes me as dubious in the common case (adding it would help very few and just be clutter for most) but not sure how many there really are to support such extensibility.