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?
Details
Details
- Reviewers
epriestley btrahan - Maniphest Tasks
- T8099: Mid-2015 Phabricator Redesign
- Commits
- Restricted Diffusion Commit
rP801607381d5e: [Redesign] PhabricatorApplicationSearchResultView
Review and do a search in each application changed. Grep for all call sites.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
A few inline ideas that you are welcome to take or leave in my opinion.
| src/applications/almanac/query/AlmanacDeviceSearchEngine.php | ||
|---|---|---|
| 83 | getResultTypeDescription() may be useful here? Its more formal than what you've put in though. | |
| src/applications/search/view/PhabricatorApplicationSearchResultView.php | ||
| 13 | Style nit - usually null members aren't explicitly set to null (like noDataString is already) | |
| 26–36 | 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? | |
| 56 | Is there a typehint we can use for $content? | |
Comment Actions
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.
| src/applications/almanac/query/AlmanacDeviceSearchEngine.php | ||
|---|---|---|
| 83 | I think it's untranslatable too. | |
| src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php | ||
| 206–209 | Maybe push this stuff into the new View too? (Possibly extend AphrontTagView.) | |
| src/applications/search/controller/PhabricatorApplicationSearchController.php | ||
| 230–262 | Consider making the ResultView actually extend AphrontView and putting all this logic into the render() method. | |
| src/applications/search/view/PhabricatorApplicationSearchResultView.php | ||
| 56 | I think this one might need to be generic, but some of the other setX(...) methods probably can have typehints, I think. | |
Comment Actions
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)
| src/applications/search/view/PhabricatorApplicationSearchResultView.php | ||
|---|---|---|
| 56 | Yeah this is generic for Calendar, Notifications, Pinboard, etc. We could maybe define them all, but seems easy to do in future if needed. | |
Comment Actions
No worries -- just kick it in and I'll counterdiff you the next time I'm in that code.