Page MenuHomePhabricator

[Redesign] PhabricatorApplicationSearchResultView

Authored by chad on Jun 17 2015, 7:50 PM.



Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?

Test Plan

Review and do a search in each application changed. Grep for all call sites.

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 32261.Jun 17 2015, 7:50 PM
chad retitled this revision from to [Redesign] PhabricatorApplicationSearchResultView.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, btrahan.
chad added a comment.Jun 18 2015, 6:11 PM

Would love to know if this was a bad direction?

epriestley edited edge metadata.Jun 18 2015, 6:15 PM

Pretty sure this is great, I just want to actually look through it before I accept.

btrahan edited edge metadata.Jun 18 2015, 7:37 PM

A few inline ideas that you are welcome to take or leave in my opinion.


getResultTypeDescription() may be useful here? Its more formal than what you've put in though.


Style nit - usually null members aren't explicitly set to null (like noDataString is already)


since you are everywhere in this diff already maybe it makes sense to make sure setNoDataString is called consistently, so you can remove this fallback code?


Is there a typehint we can use for $content?

epriestley accepted this revision.Jun 18 2015, 8:15 PM
epriestley edited edge metadata.

I think this can be just a touch cleaner if all the parts that pick pieces out of the new View just live inside it, couple of inlines to that effect. Not a big deal either way, though, and easy to clean up later.


I think it's untranslatable too.


Maybe push this stuff into the new View too? (Possibly extend AphrontTagView.)


Consider making the ResultView actually extend AphrontView and putting all this logic into the render() method.


I think this one might need to be generic, but some of the other setX(...) methods probably can have typehints, I think.

This revision is now accepted and ready to land.Jun 18 2015, 8:15 PM
chad added a comment.Jun 18 2015, 8:53 PM

Not sure how to assemble them back in the View. Dashboards and AppSearch each transform the bits in different ways, (like filters, header changes). Maybe this is above my engineering paygrade. 8)


Yeah this is generic for Calendar, Notifications, Pinboard, etc. We could maybe define them all, but seems easy to do in future if needed.

No worries -- just kick it in and I'll counterdiff you the next time I'm in that code.

This revision was automatically updated to reflect the committed changes.
chad marked 4 inline comments as done.