Page MenuHomePhabricator

Override project nav menu filter if link item matching URI exists
AbandonedPublic

Authored by epriestley on Aug 5 2017, 2:15 PM.
Tags
None
Referenced Files
F14077741: D18346.diff
Fri, Nov 22, 1:06 AM
F14074110: D18346.diff
Thu, Nov 21, 4:08 AM
Unknown Object (File)
Tue, Nov 19, 12:35 PM
Unknown Object (File)
Fri, Nov 15, 5:51 AM
Unknown Object (File)
Mon, Nov 11, 4:24 AM
Unknown Object (File)
Wed, Nov 6, 4:40 PM
Unknown Object (File)
Wed, Oct 23, 2:50 AM
Unknown Object (File)
Oct 22 2024, 3:53 PM
Subscribers

Details

Summary

Fixes T12949 for project side nav.

Test Plan
  1. Add a link item to a project side nav menu with a relative URI to a filtered view of the project workboard.
  2. Visit that link and see that it's selected in the menu.
  3. Visit other links in the menu and see that they're selected.
  4. Create that filter by hand again and notice that the relevant link item is selected.

Diff Detail

Repository
rP Phabricator
Branch
T12949
Lint
Lint Passed
Unit
Tests Skipped
Build Status
Buildable 17919
Build 24065: arc lint + arc unit

Event Timeline

I think menu selection goes somewhere inside PhabricatorProfileMenuEngine logic, so that it works with any link on any page, not just workboards. I can poke around here in a day or two.

Oh, I see. I think I explored that but got stuck trying to find an AphrontRequest object, but now I see that could come from $this->controller.

Yeah, this is in the ballpark of reasonable and should be fine locally until we get a real fix upstream (i.e., not a security risk or anything), but probably a little far afield for upstreaming. Some issues:

  • This is acting on a view, but it would be better to act at the MenuItem level so that, e.g., a third-party menu item which returns a picture of a dog (instead of a link view) can steal the selection in the future.
  • $is_link is an implicit instanceof, but it would be nice to do this without that (e.g., add a getURIForStealingItemSelection() or shouldStealSelection($request) to MenuItem). This is adjacent to the issue we currently have elsewhere where each MenuItem may generate multiple views, see T12871#228011. I'm not yet entirely sure how what the path forward out of that is (possibly, we need to "Rework the ability for configurations to generate 2+ items...").
  • This won't handle query strings, so ?order=priority (e.g., a saved link to a particular sort order) won't work.
  • As @chad notes, this only works on workboards, but it should work everywhere.
  • Ideally, it should find some more clever way to get access to $request so we don't need to remember to pass it it every time we use a ProfileMenu. If we can't find a clever way, we should probably make it required (i.e., throw if the caller doesn't pass it) and fix all the callsites.

Thank you for the feedback. I'll keep this as-is on our install and keep playing with the general solution.

epriestley abandoned this revision.
epriestley edited reviewers, added: rfreebern; removed: epriestley.

Approximately obsoleted by the very messy patch in D20353. See also T13275.