Page MenuHomePhabricator

Remove counts from home navigation
ClosedPublic

Authored by chad on Jan 20 2017, 9:28 PM.

Details

Summary

Ref T12136. This just yanks the band-aid off. Fundamentally these were useful well before Dashboards and advanced bucketing, but not so much any more. They also have some performance hit.

Test Plan

Add some tasks and diffs onto a new instance, see there is no count on the home menu bar.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chad created this revision.Jan 20 2017, 9:28 PM
epriestley requested changes to this revision.Jan 20 2017, 11:26 PM

I think we can get rid of a lot more stuff, like DifferentialApplication->loadNeedAttentionRevisions(), PhabricatorApplication->loadStatus() and callsites, the entire PhabricatorApplicationStatusView class, the stuff in People, Phrequent, Maniphest and Flags, probably some strings in USEnglishTranslation if you want to get ambitious, and the MAX_STATUS_ITEMS constant. Like seven million lines of code.

This revision now requires changes to proceed.Jan 20 2017, 11:26 PM

Maybe even some CSS!

chad added a comment.Jan 20 2017, 11:58 PM

seems like we could easily integrate this into MenuItem. I didn't know about loadStatus

chad edited edge metadata.Jan 21 2017, 12:04 AM
chad updated this revision to Diff 41457.
  • remove php code
chad added a comment.Jan 21 2017, 12:04 AM

Maybe even some CSS!

I'll leave CSS for another diff in case we have to resurrect this code after the menu swap.

epriestley accepted this revision.Jan 21 2017, 12:07 AM

Glorious!

We could definitely integrate this into MenuItem (at least, if Home is staying wide? Maybe not if it's collapsing like Projects?) but ideally maybe no one notices or misses this stuff.

If we do ultimately put it back I want to try to put some proper caching behind it -- the cost is really disproportionate to the value, especially since 80% of the value is probably "is there a count or not" for users who find it valuable?

Maybe hold this for the release cut just in case? Although I wanted to try to get some more stuff in so I dunno what's going on there yet.

This revision is now accepted and ready to land.Jan 21 2017, 12:07 AM
chad added a comment.Jan 21 2017, 12:10 AM

In pretty much all cases, I’d rather put time into fixing dashboards or
building Nuance, etc, if the count behind something is important.

Yes, I plan to hopefully have one wide menu for projects/home and roll them
out together.

Yeah, completely agreed.

Also I think I'm going to churn on some more stuff before cutting the release so just go ahead and kick this in, should have plenty of time to soak. I was just vaguely thinking I'd be cutting in like 15 minutes but I'm not going to try to rush it.

one wide menu for projects/home

happycat

Also I think I'm going to churn on some more stuff before cutting the release

uh or I'll just do it now

¯\_(ツ)_/¯

chad added a comment.Jan 21 2017, 6:46 PM

safe to land?

Yep, go for it.

This revision was automatically updated to reflect the committed changes.