This could probably use some design touches (proper disclosure triangle icons?) but maybe is reasonable?
Diff Detail
- Repository
- rP Phabricator
- Branch
- arcpatch
- Lint
Lint Warnings Severity Location Code Message Warning webroot/rsrc/css/layout/phabricator-action-list-view.css:127 TXT3 Line Too Long - Unit
Tests Passed - Build Status
Buildable 223 Build 223: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
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.
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
I applied this locally, but it's fataling on Cannot override final method AphrontTagView::addSigil()
Did I apply the patch wrong?
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.
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.
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.
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 ↗ | (On Diff #21431) | 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. |
(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
Cool. I'll fix up the tag thing and then land this a little later, I have a meeting to head to right now.
- 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? |