Ref T4430. This just deploys it on the property lists. (Help on how to do translations better? I tried a more traditional pht('%s, %s, %s, and %d other(s)') but I think the string lookup assumes the %d comes as the second param or something?)
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T4430: Summarize long lists of subscriptions and subscription changes
- Commits
- Restricted Diffusion Commit
rPc7079b52a2f3: Subscriptions - make a dialog for massive subscription lists
Made a Maniphest Task with a hojillion subscribers and noted the working dialogue. Also made a Pholio Mock with lots of subscribers and it worked.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Nice! Couple of fluff inlines.
pht('%s, %s, %s, and %d other(s)')
I"d expect that to work, but the translation needs to look like:
'blah' => array(
array(
array(
array(
'%s, %s, %s, and %d other',
'%s, %s, %s, and %d others',
),
),
),...or something like that. Maybe that's the issue?
| src/applications/subscriptions/controller/PhabricatorSubscriptionsListController.php | ||
|---|---|---|
| 30–32 | Oh yuck, are these still not on real edges? I thought I'd gotten further on this. :o | |
| src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php | ||
| 111–113 | This seems a little bit weird, maybe? Since we won't list other auto-subscribed users. But probably fine. Was there specific thinking behind it? Maybe we should make isAutomaticallySubscribed() be getAllAutomaticallySubscribedPHIDs() or something instead. | |
| src/applications/subscriptions/view/SubscriptionListDialogBuilder.php | ||
| 62 | If we just use a normal object list here, how bad does it look? Particularly, we can add profile pictures after T4400, and I'm hoping to get this into shape so we can show a list of users on the Project Members interface using standard UI. If it's okayish but just missing profile pictures, maybe do that for now and then we can pretty it up after T4400? | |
Comment Actions
on the translation, I put the '%d other(s)' into its own pht block. Since I need to make a link using the '%d other(s)' output I realized I can't do the whole thing at once and need to do it piece wise.
| src/applications/subscriptions/controller/PhabricatorSubscriptionsListController.php | ||
|---|---|---|
| 30–32 | yeah, but its the only one I think so this was really easy | |
| src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php | ||
| 111–113 | I will remove this in the update and we can do the getAll thing later maybe. When I was building this it seemed strange to have "You are automatically subscribed" in the actions and then have a subscriber list of None or see this dialog and not be present within it. Long term, I think we should do the getAll thing. (Relatedly, building the dialog I realized we'll probably want to be able to manage subscriptions from it some day, and as such making sure the list is accurate seems important. I digress though.) | |
| src/applications/subscriptions/view/SubscriptionListDialogBuilder.php | ||
| 62 | See screenshot. I am going to keep it as a normal object list since you seem to be digging at the 100 x 100 thing T4400 needs so it won't be long methinks. | |
| webroot/rsrc/css/application/subscriptions/subscribers-list.css | ||
| 32 | Thanks for the feedback - I thought this looked not right and smaller would be better. Alas @epriestley's "use an object list" feedback makes your CSS feedback no longer applicable. :/ | |