Page MenuHomePhabricator

On Dashboard tab panels in edit mode, make the "Tab Name" and the "Dropdown Edit Caret" into different links

Authored by epriestley on Apr 11 2019, 3:06 PM.



Ref T13272. In edit mode, tab panels now have a dropdown menu. However, this sort of overrlaps with the actual action of clicking the tab to select it.

Separate these into different click targets so that "select tab X" and "open dropdown menu for X" are different operations.

This is more work than it appears because:

  • We have an "action icon" already, used when you put a dashboard on a portal/home to create an "Edit" link. It makes sense to attach dropdowns to this, but it has some hard-coded stuff.
  • In applications with a "Create <thing>" in the crumbs (like Maniphest), we may use a dropdown menu if there are multiple create forms available. However, this menu renders in a weird way by reading all the properties out of an actual "View" object and building something else.
  • The "list of tabs" stuff shares code with different "list of tabs" navigation used by Diffusion and Instances.

..but I think I fixed everything and didn't break anything.

Test Plan
  • Clicked "select tab" and "open dropdown menu" as separate actions.
  • Viewed Diffusion, Maniphest with multiple create forms, Instances.

Diff Detail

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

Event Timeline

epriestley created this revision.Apr 11 2019, 3:06 PM
epriestley requested review of this revision.Apr 11 2019, 3:07 PM
amckinley accepted this revision.Apr 11 2019, 6:16 PM
amckinley added inline comments.

I'm sure there's an obvious reason why we can't make a lint rule that detects "incorrect" indentation, but it's escaping me. I know we already enforce requiring braces instead of single-line blocks... Is it just that we don't want to mandate the number of spaces in an indent, which means we'd have to detect the "intended" number before enforcing things?


haha I don't think I've ever seen a //TODO in an exception before

This revision is now accepted and ready to land.Apr 11 2019, 6:16 PM
epriestley added inline comments.Apr 11 2019, 6:21 PM

No technical reason, it's just a lot of work to write the rule because there are so many cases. Not weird special cases or anything, just a lot of actual language constructs that affect indentation so you have to enumerate all the different control statements and class and switch and try/catch and so on.


Yeah, this is a little sketchy but I didn't want to leave us with a silent failure when you configure the View in an apparently-correct way that magically doesn't work.

This revision was automatically updated to reflect the committed changes.