Page MenuHomePhabricator

Redesign Home/Profile/Projects side navigation
ClosedPublic

Authored by chad on Jan 27 2017, 7:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 9:17 AM
Unknown Object (File)
Sun, Mar 10, 9:31 PM
Unknown Object (File)
Sun, Mar 3, 11:05 PM
Unknown Object (File)
Sun, Mar 3, 11:05 PM
Unknown Object (File)
Sun, Mar 3, 11:04 PM
Unknown Object (File)
Sun, Mar 3, 11:04 PM
Unknown Object (File)
Sun, Mar 3, 11:04 PM
Unknown Object (File)
Sun, Mar 3, 11:04 PM

Details

Summary

Ref T11957. Needs some more polish, but I think everything here is square.

Test Plan

Add personal/global items to home, test mobile. Test workboards / colors.

Diff Detail

Repository
rP Phabricator
Branch
home-menu-2 (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 15411
Build 20306: Run Core Tests
Build 20305: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
src/applications/home/controller/PhabricatorHomeMainController.php
36–37

i'm not sure what this function is or how to test it.

chad planned changes to this revision.Jan 27 2017, 8:23 PM

Need to test workboard points, delete phui-profile-nav-css if not needed, fix alignment on non-icon navs items.

Not sure how much of this is still in flux, but design stuff:

  • Sub-items losing their indent jumped out at me as a little weird -- left side feels crowded now. Also, does the font get like 1px bigger when selected? Maybe I'm just crazy. This could also just be a familiarity thing, but the new version of this feels less designed than the old one did to me, at first glance. Maybe this is all totally accidental fallout from updating profile menus?

Screen Shot 2017-01-27 at 12.49.08 PM.png (447×379 px, 46 KB)

  • Since, in contrast, the actual ProfileMenu stuff looks great to me.

User profile:

  • "Home" icon on user profile item feels a little weird, use user profile icon itself instead for consistency with Projects?

Home behavior:

The 'only' stuff is when you visit /home/. This is relevant for mobile. The way this works now is:

  • On mobile, at /, you just see the left menu.
  • The top item is a link to /home/.
  • That page renders the home page without a left menu.

For example, on this install: https://secure.phabricator.com/home/ -- same as /, but no left menu.

So what we do with this depends on how you want the mobile UX to work.

The PhabricatorGlobalUploadTargetView stuff specifically avoids activating "drop files here to upload them" when you're viewing the mobile version of home. I think the behavior doesn't hurt anything, but it's not meaningful on mobile.

Home Content:

  • We've lost the link to "All Applications", so there's pretty much no way to get to that list now?
  • We've lost all the hints about application function ("Code Review", "Browse Repositories", etc).
  • Device menu is currently wider than non-device menu? Works for phones but maybe wrong behavior for tablets?
  • Devices have a duplicate of everything in the three-bars action menu, maybe fine?
  • No apparent way (?) to get to the actual Home content on Mobile now.
src/applications/home/controller/PhabricatorHomeMainController.php
36–37

(Added some notes in main comment.)

src/applications/home/menuitem/PhabricatorHomeManageProfileMenuItem.php
54–55

Redundant assignments to $href, remove first line?

src/applications/people/controller/PhabricatorPeopleProfileManageController.php
26

Maybe try to lift this stuff up to the parent if you can, since we copy/paste it in two places now?

("Administrator" text is bold on "Manage" and nonbold on "Home" for me, CSS leak?)

src/applications/people/menuitem/PhabricatorPeopleDetailsProfileMenuItem.php
52 ↗(On Diff #41517)

Oh, this was an explicit change? Feels inconsistent with projects now? Maybe fa-user instead of fa-home if there's some other motivation here? I don't really feel strongly about this.

src/view/layout/AphrontSideNavFilterView.php
206–207

(Nuke?)

webroot/rsrc/css/phui/workboards/phui-workboard.css
35

Do we still use this variable anywhere?

fix alignment on non-icon navs items.

oh well then

reading is fundamental

🌈

the nav spacing was my comment above "fix alignment on non-icon navs items."

I feel good when design stuff which feels a little off to me is really just unfinished and not some kind of next-level cutting-edge new-age design which I don't "get".

💈 💈 💈 💈 💈 💈 💈 💈 💈 💈 💈

💈 This comment brought to you by Design Stripes™. 💈

How did we want to explore renaming?

Some options I can come up with are:

  • Do nothing.
  • In PhabricatorHomeProfileMenuEngine->getBuiltinProfileItems(), map 'name' to getNameForPinnedApplication() or something, which returns getName() by default. Then implement / override that method for Differential to return "Code Review", Maniphest to return "Tasks", etc., so the home page items say "Code Review" instead of "Differential" but everything else is the same.
  • Actually rename "Differential" to "Code Review", including all the classes, all the URIs, all the Conduit API methods, etc.

I personally tend to favor doing nothing. I think the names aren't a root problem: Atlassian and AWS both have application names like we do (Atlassian "Confluence" is a wiki, "Route 53" is a DNS service) and are industry leaders. I think Apple is almost the only major company that does not do this (maybe?), and even they do in some cases ("Garage Band", "Final Cut", "XCode", "Safari"). I think complaints about this are relatively rare and they are symptoms of other issues, like overall roughness in NUX and users just not liking new stuff and looking for something to complain about. Administrators can also perform this rename themselves if they want.

GitHub does not have application names, but it does far less than Phabricator does. And it actually does have application names: Gist is not "GitHub Text Files", but a named application with some wordplay in the name.

Even GitLab now has "Pipelines", not "Builds" and "GitLab Geo", not "Gitlab Server Clustering".

Swapping the names is an easy step to leave hints in place, but I think renaming "Differential" to "Code Review", but then linking to a page that says "Differential", risks making confusion worse. We could make it say "Differential (Code Review)" but that's feels kind of questionable and ugly to me. We could maybe go half this far and add tooltips or something on Home, or iterate on the design to get a second line of text back. But I'm not sure those are worth pursuing.

Fully renaming things is possible, but will take a huge amount of effort and be tremendously disruptive and confusing to existing users. I don't think any name for some applications, like Herald, Spaces, or Drydock would make them more clear. I think there are a bunch of other good arguments for application names, and the only argument against them is basically "sometimes new users write angry tweets about them". It's definitely possible that this is legitimately a root issue that's turning away a tremendous amount of adoption, but my gut is that it's fluff.

(Offhand, I don't think I've seen these tweets from users who were clearly administrators who installed Phabricator: the ones I can remember felt more like users who didn't set it up themselves, and presumably were told to go use it by someone else? Maybe I'm misremembering.)

Two half-way compromises are probably the "Differential (Code Review)" or what I think I mentioned earlier, which was using object names (Tasks, Diffs, Documents) instead. I'll probably just go with "Differential (Code Review)" which is at least a more concise version of the current navigation we offer and users can change it from there.

Mostly, I think this is inconsistent UX on our side. We're pretty good about making sure things are easy, well explained, and obvious. Things like the action menu, configuration, setup errors, are all very verbose, obvious, and simple to understand. I do understand all your points, but keep coming back to the very basic design experience principle of "Don't make me think".

I feel though like all these ideas aren't great. Like I don't think most need further explanation.

Maybe I'll run an unscientific social media poll.

Leaning towards:

  • Application (Object)
  • Application (Large JavaScript Tooltip that blinks and takes 5 seconds to load)

Truth be told I have a diff locally that removes serious business more for all boring all the time.

pasted_file (128×728 px, 26 KB)

exciting week for me on twitter

  • add Launcher
  • fix some colors, spacing
  • remove unused CSS file
  • remove unused celerity variables
  • re-align navigation items
chad planned changes to this revision.Jan 28 2017, 5:18 AM

Not done, just pushing up for the night.

chad marked 5 inline comments as done.
  • route non-admin to correct menu
  • make profile header a top level function
  • add back profile picture

TODO:

  • Test points
  • Selected state CSS not being applied correctly.
  • Dashboards don't auto-load on /home/
In D17259#205427, @chad wrote:

Truth be told I have a diff locally that removes serious business more for all boring all the time.

Noooooooo!

  • clean up phui-segment-bar-view
  • Add tooltip for description to Applications

Tooltips are in, but the animation is janky and our tooltips flake on and off dragging the mouse around the element. Would like some assistance in making these appear like butter if we're going to have them there.

I can counter-diff you for tooltip smoothness, hard to fix before this lands. Here are the issues I identified, and how I fixed them:

  • Webkit antialiasing pop-in at the end of the animation: in Safari, there's a jarring pop-in effect when the animation ends and Safari changes antialiasing. I added an explicit -webkit-font-smoothing: subpixel-antialiased; to the CSS, which seemed to clean this up.
  • Flicker inside elements: mousing around inside an element would fire mouseover and mouseout effects and replay the tip animation.
  • Flicker between elements: mousing between elements would replay the tip animation. This wasn't as broken-looking, but meant you had to wait 250ms to read each tip.
    • I fixed both flicker issues by recording when the last tip was hidden. If it was hidden less than 500ms ago, we do not replay the intro animation and just show the tip immediately. The tip is now redrawn instantly in-place during mouse events, and the animation does not replay when moving up and down the menu.

Other stuff:

  • I think dashboards are rendering on-page correctly, but PhabricatorHomeMenuItemController calls setShowNavigation(false), which hides the navigation for all routes. Maybe just showing navigation here makes sense, as an easy fix (like Projects)?
  • The /home/ route fatals. If it isn't being used to serve mobile home, we should just remove it from the routes in HomeApplication.
  • Still no way to get to default home from mobile?
src/applications/people/controller/PhabricatorPeopleProfileController.php
73–88

This will produce misleading results, because users may have multiple of these roles but only one is shown.

For example, a "Disabled + Not Approved" user will show only "Not Approved", which I think is misleading. (Of the two, "Disabled" is much more important.) Likewise, a Disabled "Bot" or "Mailing List" will omit "Disabled". If we must choose only one role to show, "Disabled" is the most important role to show.

An "Administrator + Bot" user will show only "Bot". I think this is a significant omission.

src/applications/project/menuitem/PhabricatorProjectPointsProfileMenuItem.php
80

Missing pht().

Also, kind of weird that these tooltips show up for all Application menu items, including in Projects. Maybe consider a rule like this:

  • If the item has a custom name, do not show the tip. For example, if a user renames their "Differential" link to "Code Review", assume the name is meaningful and sufficient and does not require an additional tooltip.
  • If the item is not builtin ($config->getBuiltinKey() is null), do not show the tip. For example, if a user explicitly adds "Slowvote", assume they know what the application does.

we could probably allow custom tooltips as well

dereckson retitled this revision from Redesign Home/Profie/Projects side navigation to Redesign Home/Profile/Projects side navigation.Jan 30 2017, 5:27 PM

It would be fairly easy technically, although I'm not sure it would see much use (if the item is custom, presumably you just give it a meaningful name?). Any information in a tooltip won't currently be available on mobile, too (and I don't see an easy way to make it available, at least without design changes).

It also doesn't work in the Favorites menu (at least, not yet) and would be a little weird if it did (maybe?) so it would either need to work there or not be an option there, which means editing needs to become context-sensitive. That is, if we add a "Tooltip: ..." field to the ApplicationItem, but it has no effect in the Favorites menu, that's confusing. We could fix that by making the ApplicationItem "know" that it's in the Favorites menu so it doesn't offer you a "Tooltip" field, or by making tooltips work in the menu (but: weird?).

If we do make it context sensitive, it gets a little weird that extensions won't be able to anticipate new contexts: if you write a custom item today, you'd have to hard-code it to do something weird (and probably something that "knows" about the Favorites menu) if you want it to have tooltips. It would be better if custom MenuItems didn't need to have any knowledge of the Favorites menu, so they don't break when we introduce "Favorites v2" or "Personal Efficiency Console Menus" or "Phriction CMS" menus or Phame blog menus or whatever. We're already exposed to this a little bit, and there are some remedies for it, but they increase complexity more.

chad marked 2 inline comments as done.
  • Add Home menuitem
  • Fix profile roles
  • Hide tooltips if name is customized
  • Fix mobile route fatal
  • pht a string

Still having "selected menu item" issues, but haven't looked into it yet. Home is hardwired on global, so it doesn't just magically appear on mobile which I think is slightly better for me at least, since we're adding Dashboards. I assume maybe even we can remove the "Install to Home" action, since you can just add any Dashboard and (eventually) drag it to the top. This will stop people from "losing" the default we ship with?

Yeah, I think we should get rid of "Install to Home" and just write a blog/guide about upgrading -- I think this menu mechanism is just better/clearer/more flexible in pretty much every way. We could maybe migrate existing installed dashboards to become menu items automatically, but that doesn't feel tooooo necessary since we're shipping it alongside new stuff and I think most installs will be happier to play with the new stuff than frustrated by needing to reconfigure a home dashboard.

(If we did want to replace it, the flow could go "Install to... > choose a project / favorites / home > choose global / personal > it adds an item for you", but I'd like to see evidence that "Edit Menu Items > Dashboard" isn't clear enough before we build that.)

i do want to replace the flow, but as part of Dashboards / Search Action UX... it can come later.

I think I should go ahead and build a migration now for this though, change everything in one swoop.

can you once over the migration script for me?

resources/sql/autopatches/20170131.dashboard.personal.01.php
30–35 ↗(On Diff #41536)

specifically, will this run and build an object?

Maybe somewhat helpful?

resources/sql/autopatches/20170131.dashboard.personal.01.php
6–11 ↗(On Diff #41536)

This will come back null if Home is uninstalled, e.g. in the Phacility cluster. Then, we'll fatal on line 31. We should probably just migrate no matter what. One advantage is that we won't lose anything if someone uninstalls home, then migrates, then installs it again later. You can get the PHID like this, without worrying whether the application is installed or not.

$home_phid = id(new PhabricatorHomeApplication())
    ->getPHID();
13 ↗(On Diff #41536)

It would be nice to use LiskRawMigrationIterator here (which returns raw rows, instead of objects), instead of LiskMigrationIterator, so that we can delete PhabricatorDashboardInstall later without revisiting this migration. Right now, we have to keep PhabricatorDashboardInstall around forever.

16 ↗(On Diff #41536)

Should be unnecessary.

21 ↗(On Diff #41536)

I think $dashboard_install is never defined?

22 ↗(On Diff #41536)

We probably don't need to load the panels, I think. Do we even need to load the dashboard at all? Can we just use the PHID?

24 ↗(On Diff #41536)

It's possible that $dashboard will come back null, if someone deleted a dashboard manually or maybe used bin/remove destroy. Slightly cleaner to test for !$dashboard and just continue;.

35 ↗(On Diff #41536)

This doesn't actually save() the object so I think it won't do anything.

Ideally, we would do a raw INSERT instead of a save(). With save(), this migration may break in the future if we change the table. For example, if we later add an icon field to the Configuration, this save() will start trying to INSERT it, but it won't exist yet because this will run before 20180913.add-an-icon-to-menu-items.sql or whatever, which actually adds the column.

chad marked 8 inline comments as done.
  • per comment

Looks pretty close. You can run this migration locally to test it like this:

./bin/storage upgrade -f --apply phabricator:20170131.dashboard.personal.01.php

Every time you run it it will add another item to the menu, but that's fine. Should be fine to run a ton of times until it works well since it doesn't delete anything.

resources/sql/autopatches/20170131.dashboard.personal.01.php
18 ↗(On Diff #41544)

I think this'll fatal now since $install is an array, not an object, so ->getDashboardPHID() isn't valid.

(You probably don't need to load the dashboard at all? It's OK to insert a bad menu item, we'll just say "Invalid dashboard".)

37 ↗(On Diff #41544)

I don't think this syntax is valid -- it's a mixture of UPDATE and INSERT. Should be:

INSERT INTO %T (col, col, col, ...) VALUES (%s, %s, %s, ...)

I ran it and I think nothing broke. Can you test it?

  • If you also insert a 0 into menuItemOrder, the items will appear at the top of the menu instead of the bottom.
resources/sql/autopatches/20170131.dashboard.personal.01.php
17 ↗(On Diff #41545)

For consistency, prefer $dashboard_phid vs $dashboardPHID.

31 ↗(On Diff #41545)

The last one here should be %ns ("nullable string"), not just %s ("string"), so that we insert null if $custom_phid is null. As is, this will migrate personal dashboards but not global dashboards.

chad marked 2 inline comments as done.
  • remove install

Yeah, sending them to MenuItem to add their new dashboard is kind of a crap experience, given there are hundreds of thousands. Hard to find ones you made.

I think T12174 captures everything I'm aware of, I can shoot you followups for some of it. Do you want to kick this in as-is and then we can fix it up on T12174?

This revision is now accepted and ready to land.Jan 31 2017, 4:18 PM
This revision was automatically updated to reflect the committed changes.