Fixes T12949 for project side nav.
Details
Details
- Reviewers
rfreebern - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T12949: If a ProfileMenu has Link items, and one or more have the request URI as the link target, highlight the first one
- Add a link item to a project side nav menu with a relative URI to a filtered view of the project workboard.
- Visit that link and see that it's selected in the menu.
- Visit other links in the menu and see that they're selected.
- Create that filter by hand again and notice that the relevant link item is selected.
Diff Detail
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
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
Thank you for the feedback. I'll keep this as-is on our install and keep playing with the general solution.