Page MenuHomePhabricator

Redesign header menus and search
ClosedPublic

Authored by chad on Jan 14 2017, 8:18 PM.
Tags
None
Referenced Files
F14057679: D17209.diff
Sun, Nov 17, 5:50 AM
F14030801: D17209.id41387.diff
Sat, Nov 9, 6:50 AM
F14028041: D17209.id41386.diff
Fri, Nov 8, 11:31 AM
F14027999: D17209.id41394.diff
Fri, Nov 8, 11:16 AM
F14027996: D17209.id41395.diff
Fri, Nov 8, 11:16 AM
F14027994: D17209.id41401.diff
Fri, Nov 8, 11:16 AM
F14027992: D17209.id41391.diff
Fri, Nov 8, 11:16 AM
F14017249: D17209.id.diff
Mon, Nov 4, 3:22 PM
Subscribers

Details

Summary

Still lots to fix here, punting up since I'm running into a few roadblocks.

TODO:

  • Sort Personal/Global correctly
  • Quicksand in Help Items correctly on page changes
Test Plan

Verify new menus work on desktop, tablet, mobile. Test logged in menus, logged out menus. Logging out via a menu, verify each link works as expected. Help menus get build when using an app like Maniphest, Differential. Check that search works, preferences still save.

Diff Detail

Repository
rP Phabricator
Branch
menu-redesign (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 15283
Build 20108: Run Core Tests
Build 20107: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Remove QuickActions
  • Add divider to Favorites as builtin

pasted_file (292×774 px, 39 KB)

not sure why it said I edited the description on the transaction above ^^

  • Fix login/logout
  • Remove phabricator-dark-menu
  • make help links open in new window
  • Fix search mobile layout
  • Normalize drop menu border colors

Calling it a night, feel like this is 90% there, but I need to fix a few more bugs up on the list. Feel free to review stuff that's obviously wrong.

chad edited the summary of this revision. (Show Details)
chad edited the summary of this revision. (Show Details)
  • move application menu to phui-button-view

Tried to fix the Quicksand issue, have it redraw the PhabricatorHomeApplication menu each pass, but it pops an Exception Attempting to add more metadata after metadata has been locked. when drawing the dropdown actionlistview. I'm not sure how to resolve that.

Help stuff feels a little weird in this menu but I guess it's the best place to put it.

Would be nice to have icons, maybe? But given the removal from remarkup I guess that's intentional and you want to move away from icons in menus?

Maybe fa-bookmark (or -o) instead of fa-star-o? Also resolves T12107. Meaning of star isn't immediately obvious/intuitive to me. Not sure bookmark is really better, of course. Also, other icons are non-outline (notification, chat). Not obvious what outline means or why this one is outlined. My initial guess is maybe that it's an outline because it's empty, e.g. nothing in the menu? But I assume the real reason is just "it looks better"?

Edit UIs are kinda funky because they show the menu on the left, and both have an "Edit Favorites" item.

I'll look at the quicksand thing...

src/applications/favorites/application/PhabricatorFavoritesApplication.php
39

Should logged out have Favorites?

Probably show global items only?

75

Does the wrong thing for items named "0".

src/applications/favorites/engine/PhabricatorFavoritesProfileMenuEngine.php
62–71

This should only be part of the global menu, I think. Currently, you get an item on each of "Edit Personal" and "Edit Global".

Once you load global + personal you'll get two items, I believe.

src/applications/home/application/PhabricatorHomeApplication.php
91–92

If $application isn't a valid aplication, we'll fatal on line 90 when calling $application->getHelpMenuItems() (and, in fact, when invoking this function, since it has a PhabricatorApplication typehint), so this if ($application) check on 91 can't ever do anything, I think?

123

This reads a little weird to me. Maybe Profile ("%s") or something instead, if the goal is to show which account you're logged in as? That seems a little weird too. Maybe an unclickable "Logged in as %s"?

Or put icons on all this stuff and do:

[ your profile icon ] <epriestley>

...as the first item in the menu, which is either unclickable or takes you to your profile (replacing "profile")?

src/view/phui/PHUIButtonView.php
91–92

"puppet"?

src/view/phui/PHUITimelineEventView.php
657

This isn't really information, exactly, but it is now conveyed only through the use of color.

I like this menu less without icons.

webroot/rsrc/css/application/base/main-menu-view.css
553

woah woah there's no nintendo IP in this codebase and any resemblances of any profile icons to any characters, living or dead or fictional Pokemon, are purely coincidental

webroot/rsrc/css/phui/phui-button.css
209

Remove if unused?

I can't repro any errors with Quicksand enabled, presumably that's after additional changes?

In general, what's probably happening is this:

  • We probably build footer metadata before actually rendering the menu.
  • So, when we render the menu, it's too late to add more stuff to the metadata.

The fix is probably to force the menu to render earlier, then drop the rendered nodes in later. But I'm not exactly sure.

It might look something like this:

- $body = $this->buildThePageBodyNormally($menu);
+ $rendered_menu = hsprintf('%s', $menu);
+ $body = $this->buildThePageBodyNormally($rendered_menu);

But hard to say.

src/view/page/menu/PhabricatorMainMenuView.php
7

Unused?

src/view/phui/PHUIButtonView.php
91–92

No puppet, No puppet. I'm not the puppet, you're the puppet.

chad marked an inline comment as done.Jan 16 2017, 8:45 PM
chad added inline comments.
src/view/phui/PHUITimelineEventView.php
657

uuuuuuuuuuugh.

pasted_file (728×1 px, 111 KB)

i can fix that maybe

if ur lucky

chad marked 4 inline comments as done.
  • give back icons to edit menu cause epriestley is good boy, brint
  • make favorites work logged out
  • remove FavoritesManageMenuItem, not needed
  • move Edit Favorites into Favorites Application

I removed the icons in the header mostly because I found them to be distracting / useless. It might be an our install thing, but I'm betting not. Having three items for "new task", "new bug", "new feature" is a lot easier for me to read and find without identical icons for each. So at least for now, I decided not to include icons into the Favorites menu. I think though over time once we get a better sense of how we're using it, we'll spruce it up (ie, project images, etc).

On this install, I'd expect to delete most of that menu and pick new icons:

New Feature Request
New Bug Report

Don't think we have an icon typeahead

I'd lean towards putting that onto EditEngine itself, so that wherever we use those form name/icon it's consistent.

chad marked 8 inline comments as done.
  • fix rest of comments
chad edited the test plan for this revision. (Show Details)
chad edited the test plan for this revision. (Show Details)
  • better blank states for favorites
  • add ability to not show page navigation on editengine / menu item

Quicksand doens't work for me (the menu doesn't update), but it doesn't break either -- are you seeing actual breaking stuff? If it just doesn't work, I can counter-diff you to fix it up after this. I can probably just fix it faster than I can give you pointers, since I pretty much have no clue where to start.

I think Tthe personal/global stuff is because the "global" query is still loading all personal items for all users, too, because it never adds customPHID IS NULL to the query.

Currently, PhabricatorProfileMenuEngine->loadItems() has two modes:

  • With customPHID: load items with profilePHID = %s AND customPHID = %s (correct).
  • Without customPHID: load all items with profilePHID = %s, including items with any customPHID (wrong: loads all custom favorites for all users).

It should really have three modes:

  • "Edit Personal Items": User is editing their personal items. Load just profilePHID = %s AND customPHID = %s.
  • "Edit Global Items": User is editing the global items. Load profilePHID = %s AND customPHID IS NULL.
  • "Show The Menu": User is looking at the menu. Load both, then reorder them as user favorites, then global favorites.
    • You could do two queries, or make the Query fancier and do one query like profilePHID = %s AND (customPHID IS NULL OR customPHID = %s).
src/applications/favorites/application/PhabricatorFavoritesApplication.php
83–86

Maybe make /favorites/ do this redirect? As a non-admin, I can go there now and see "Edit global stuff", but then I get an edit error.

If /favorites/ just sent non-admins directly to /personal/.../ that might be a little cleaner.

src/view/phui/PHUIListItemView.php
205

I think there's maybe some magic on AphrontTagView which makes this unnecessary? Not sure.

chad marked 2 inline comments as done.
  • changes per comments

Still working out kinks on trying to build this with one query. Two queries seems so much easier. lol.

src/applications/home/application/PhabricatorHomeApplication.php
91–92

Should be fixed.... I think.... feels jank, but 404 page works now.

123

I plan to follow up with more UI around who is logged, but I've added a quick header to it for now.

src/view/phui/PHUITimelineEventView.php
622

frowncat

I can shoot you a followup to merge 'em if you want to do the two-queries way. One query is slightly cleaner but this isn't a big performance issue like "N+1" stuff is.

Throwing in the towel on the queries. Sorry, I can't really wrap my head around a correct way to do this. It seems to sort of work, but Ideally I want:

  • Personal Items
  • Divider (hardwired)
  • Global Items
  • Divider (hardwired)
  • Edit Favorites

I'm just down to getting the queries correctly returned and I have for example, have personal items in the global editor. I'm pretty sure I just need another query for NULL there, which itself is easy, but I don't know how to build it out in QueryWorldPHP.

This should be OK to land, I don't have any fatals I can find, just need 2 counter diffs.

Sounds good to me. I'll get you followups for One Fancy Query and Quicksand doc links.

This revision is now accepted and ready to land.Jan 17 2017, 8:10 PM

31 local commits. I think that is a new record.

This revision was automatically updated to reflect the committed changes.