Page MenuHomePhabricator

Action submenus
AbandonedPublic

Authored by epriestley on Apr 3 2014, 1:03 AM.
Tags
None
Referenced Files
F14089447: D8685.id.diff
Sun, Nov 24, 10:42 AM
Unknown Object (File)
Sat, Nov 23, 1:48 AM
Unknown Object (File)
Sat, Nov 23, 1:48 AM
Unknown Object (File)
Sat, Nov 23, 1:48 AM
Unknown Object (File)
Sat, Nov 23, 1:48 AM
Unknown Object (File)
Sat, Nov 23, 1:46 AM
Unknown Object (File)
Sat, Nov 23, 1:45 AM
Unknown Object (File)
Sat, Nov 23, 1:32 AM

Details

Reviewers
btrahan
chad
Summary

This could probably use some design touches (proper disclosure triangle icons?) but maybe is reasonable?

Test Plan

Screen_Shot_2014-04-02_at_6.01.21_PM.png (697×1 px, 124 KB)

Screen_Shot_2014-04-02_at_6.01.24_PM.png (697×1 px, 127 KB)

Diff Detail

Repository
rP Phabricator
Branch
arcpatch
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningwebroot/rsrc/css/layout/phabricator-action-list-view.css:127TXT3Line Too Long
Unit
Tests Passed
Build Status
Buildable 227
Build 227: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Rough draft for action submenus.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: chad, btrahan.

Have you considered a dropdown button in the header? Or are these Events?

These aren't added by an event, but the three below them are ("View Revisions", "View Tasks", "View Commits") and those seem reasonable to group, too. We could make events add buttons, of course.

If we go down that route, how do you imagine mobile working? I'm a little worried we're going to end up with like 3-4 mystery meat buttons in some applications, maybe? Like, if we group "Admin", "View" and "Other Actions", do we get three buttons? Do they fit? What icon can we use to clearly communicate "Administrate User"?

(These questions probably have reasonable answers, I just hate writing Javascript and dropdown menus.)

Do you want to give me like a crude napkin sketch of your vision on desktop/mobile and I'll take a crack at implementing it? I can probably make pretty much anything work from a technical perspective, we don't have too many constraints here.

but like not 3D OK? I don't want to write that much Javascript

2D only

yeah let me sleep on it. this isn't urgent or anything.

3d stuff is all css now anyways

(code looks good, but my understanding is we're holding for design.)

Yeah, mostly I just took us to new heights of badness with the number of actions on user profiles (overall, I think the changes which did that are really good, but they've exacerbated an unrelated, preexisting problem). I just wanted to pay some dues for that by kicking T4426 forward a notch.

Having built a prototype of this now, it's not as nice as I'd imagined in T4426. I think it's definitely worth exploring a bit more to find better approaches. This isn't an urgent problem, but will probably just get worse over time.

So short design opinion:

  • Overall, rather not build sub-menu support, as it likely will encourage bad behaviour.
  • The "View Commits", "View this", "View that" aren't particularly action-like, or needed in the AppSearch world. Consider removal.

I'd move forward in one of two ways:

  • Move the admin stuff to a dropdown button in the header (use a gear or person icon). This should just work on mobile (it collapses to an icon with a dropdown arrow)
  • Or make a small like divider above the "admin" actions in the list and remove the "View" actions
chad edited reviewers, added: epriestley; removed: chad.

yoink

I love how the flag icon says both "Stake ownership claim" and "I surrender"

I applied this locally, but it's fataling on Cannot override final method AphrontTagView::addSigil()

Did I apply the patch wrong?

Hrrm, let me give you a clean patch against HEAD...

Try this one?

https://secure.phabricator.com/differential/diff/21266/

Should apply with:

arc patch --diff 21266

I "fixed" it in a hacky way but we can clean that up before moving forward if we come up with some approach here which seems like a reasonable direction.

chad edited edge metadata.
  • Update styles

So this is closer, I still want to add a caret to represent open/closed on the parent, but not clear the best place to put that.

  • Add dropdown caret css
chad retitled this revision from Rough draft for action submenus to Action submenus.May 3 2014, 11:38 PM
  • asdf
  • Update styles
  • Add dropdown caret css
  • Mobile tweaks

Welp, I rebased and now it's fataling again on AphrontTagView. Not sure what's going on.

Did you have feedback on this? It was ready for review.

btrahan edited edge metadata.

I caught an inline typo, have inline comments on how to fix the fatal and the new ones that will emerge =D , and some css nitpickiness on color constants.

I locally patched and made the fatal fixes so they should work.

I think this looks pretty good! I think maybe we might want a quick slide animation - something that takes like .25 seconds - but I'd rather ship this as is and see what people think about it before getting too fancy.

Feel free to send up for review again post fatal fixing or anything else if you want more feedback.

src/applications/people/controller/PhabricatorPeopleProfileController.php
68

Admisistration is the worst

src/view/layout/PhabricatorActionView.php
14–35

deleting these two functions will fix the first fatal.

(This diff changes this class to extend AphrontTagView, which has setMetadata defined already as "final" whereas AphrontView had no such definition. In object oriented programming, methods marked "final" cannot be overridden by inheriting classes. I peeked and getMetadata is also final in AphrontTagView so minimally those two have to go. Once you remove them you may have even more fatals but they should be mechanical to remove like this.)

14–35

delete me too as fatal fixing

167–168

change to $this->getMetadata() since the local member variable is now deleted

176–177

as above

webroot/rsrc/css/core/core.css
92

tangential, but here's that link color that could be a color constant I mentioned.

webroot/rsrc/css/layout/phabricator-action-list-view.css
68

no color constant for this and a few others?

While I'm being a jerk on CSS color constants, I noticed in another diff a bit ago we don't have the hyperlink color as a constant.

This revision is now accepted and ready to land.May 9 2014, 7:04 PM

(Do you think this is a good element? I don't want to railroad it through on the strength of my belief that submenus are probably a good fix here if you aren't sold after fixing it up.)

It's a good element, I may further tweak it, but there is value in have in (like admin actions, which seems to be increasing).

You can commandeer back

epriestley edited reviewers, added: chad; removed: epriestley.

Cool. I'll fix up the tag thing and then land this a little later, I have a meeting to head to right now.

I'd +2 some smooth quick blind action on the menu.

plustwo

epriestley edited edge metadata.
  • Working rebase, I'll hold this until after D9052 since it will probably need a little adjustment.
src/applications/people/controller/PhabricatorPeopleProfileController.php
79

?

src/applications/people/controller/PhabricatorPeopleProfileController.php
79

?

src/applications/people/controller/PhabricatorPeopleProfileController.php
79

?

This is probably obsolete with "Manage" pages and Curtains views. No need for it to set here collecting upwards of 1 "?" per year, I think?

src/applications/people/controller/PhabricatorPeopleProfileController.php
79

I was referring to the spacing / alignment issues of the highlight in Differential on the third ?

Was that not obvious?