This could probably use some design touches (proper disclosure triangle icons?) but maybe is reasonable?
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"?
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.
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
Try this one?
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.
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.
Admisistration is the worst
delete me too as fatal fixing
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.)
change to $this->getMetadata() since the local member variable is now deleted
tangential, but here's that link color that could be a color constant I mentioned.
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.
(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.)