Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement teams dashboard widget #1510

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Feb 20, 2024

Resolves #1506

Screenshot 2024-02-20 at 15 31 56

@marcoambrosini @jancborchardt I used the generated avatars as they are also used in contacts right now

Needs upstream adjustments in the dashboard API:

Follow up requiring server / API adjustments

  • Add icon to add button
  • Add empty content placeholder text through API
  • Show action button if not all list item slots are filled

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The avatar placeholders only fit for individual people, in this case it's better to just use the teams icon for each team.
Yes it's some duplication, but also helps to have a recognizable icon everywhere.

Also (apart from the vertical alignment you noted), the icons should be horizontally aligned below the heading icon of the widget, just like the text should be left-aligned with the heading text. Currently everything is a bit shifted to the right.

@juliusknorr
Copy link
Member Author

The avatar placeholders only fit for individual people, in this case it's better to just use the teams icon for each team.

We currently don't have one. Once the stretch goal #1508 is there we can do that, but for now this is based on what we have there in the existing circles UI already:

Screenshot 2024-02-20 at 18 20 01

Also (apart from the vertical alignment you noted), the icons should be horizontally aligned below the heading icon of the widget, just like the text should be left-aligned with the heading text. Currently everything is a bit shifted to the right.

Noted, will check how we can address that generally (as the other widgets show the same issue)

@juliusknorr
Copy link
Member Author

Quick note on the button, currently we only have options to specify a more text and a button when the list is empty (please ignore the missing icon):

empty more
Screenshot 2024-02-20 at 20 36 55 Screenshot 2024-02-20 at 20 36 33

@jancborchardt Is that enough? Otherwise we need a generic design spec on how list widgets should handle this as I'd like to keep the implementation without custom frontend implementation to account for the dashboard load performance improvements that this brings and also make sure it works on client integrations.

@marcoambrosini
Copy link
Member

@juliushaertl could we use the empty content component with an add icon and an explanation of what a team is?

@juliusknorr
Copy link
Member Author

juliusknorr commented Feb 22, 2024

Yes, though we need to extend the server API for that, would follow up in separate PRs then to keep things moving

Took notes in the description to collect that

@ArtificialOwl
Copy link
Member

Do we need to display all circles or only the one current user is member of ?
It might be heavy on resources to use getCircles() from the dashboard. This method should only be used from the contact app to display all available/visible circles.

probeCircles() is lighter and will returns all circles current user is a member of.

Now, if we display circles based on membership, we can also imagine:

  • create a newfield in circles table to store a 'last_event' value in database,
  • update last_event when a new share to circle is created/updated (or any other event),
  • add a way to order by 'last_event' when calling probeCircles() (DataProbe)

The result would be that top of the list are showing circles with more recent activity

@juliusknorr
Copy link
Member Author

Do we need to display all circles or only the one current user is member of ? It might be heavy on resources to use getCircles() from the dashboard. This method should only be used from the contact app to display all available/visible circles.

probeCircles() is lighter and will returns all circles current user is a member of.

Ah thanks, I switched that as we're only interested in the ones where a user is member

Now, if we display circles based on membership, we can also imagine:

* create a newfield in circles table to store a `'last_event'` value in database,

* update `last_event` when a new share to circle is created/updated (or any other event),

* add a way to order by `'last_event'` when calling `probeCircles()` _(DataProbe)_

The result would be that top of the list are showing circles with more recent activity

Won't get to that in this iteration, we can track this as a follow up of course as it generally makes sense I think.

@juliusknorr
Copy link
Member Author

@ArtificialOwl Mind to double check the fixup commit for the prope adjustment? I'm also not sure what effect the details have but not requesting any also gives us enough information i think.

@juliusknorr juliusknorr requested a review from mejo- March 4, 2024 12:41
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr dismissed jancborchardt’s stale review March 4, 2024 21:47

Resolved and follow ups

@juliusknorr juliusknorr merged commit ef4da38 into master Mar 4, 2024
20 checks passed
@delete-merged-branch delete-merged-branch bot deleted the enh/teams-dashboard branch March 4, 2024 21:47

private function getTeamPage(): string {
return $this->urlGenerator->getAbsoluteURL(
$this->urlGenerator->linkToRoute('contacts.page.index')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaks when the contacts app is not enabled.
Circles is default enabled, but the contacts app is not even shipped:
https://github.com/nextcloud/server/blob/60681fb8240843b294db02593811a9c600b27335/core/shipped.json#L54

Technical details

    Request ID: 1WWtRvraU7IKjR7k0KyE
    Type: ReflectionException
    Code: -1
    Message: Class "OCA\Contacts\Controller\PageController" does not exist
    File: /var/www/html/lib/private/Route/Router.php
    Line: 469


Trace

#0 /var/www/html/lib/private/Route/Router.php(469): ReflectionClass->__construct()
#1 /var/www/html/lib/private/Route/Router.php(161): OC\Route\Router->getAttributeRoutes()
#2 /var/www/html/lib/private/Route/Router.php(400): OC\Route\Router->loadRoutes()
#3 /var/www/html/lib/private/Route/CachingRouter.php(65): OC\Route\Router->generate()
#4 /var/www/html/lib/private/URLGenerator.php(103): OC\Route\CachingRouter->generate()
#5 /var/www/html/apps/circles/lib/Dashboard/TeamDashboardWidget.php(153): OC\URLGenerator->linkToRoute()
#6 /var/www/html/apps/circles/lib/Dashboard/TeamDashboardWidget.php(123): OCA\Circles\Dashboard\TeamDashboardWidget->getTeamPage()
#7 /var/www/html/apps/dashboard/lib/Controller/DashboardController.php(78): OCA\Circles\Dashboard\TeamDashboardWidget->getUrl()
#8 [internal function]: OCA\Dashboard\Controller\DashboardController->OCA\Dashboard\Controller\{closure}()
#9 /var/www/html/apps/dashboard/lib/Controller/DashboardController.php(73): array_map()
#10 /var/www/html/lib/private/AppFramework/Http/Dispatcher.php(232): OCA\Dashboard\Controller\DashboardController->index()
#11 /var/www/html/lib/private/AppFramework/Http/Dispatcher.php(138): OC\AppFramework\Http\Dispatcher->executeController()
#12 /var/www/html/lib/private/AppFramework/App.php(184): OC\AppFramework\Http\Dispatcher->dispatch()
#13 /var/www/html/lib/private/Route/Router.php(338): OC\AppFramework\App::main()
#14 /var/www/html/lib/base.php(1065): OC\Route\Router->match()
#15 /var/www/html/index.php(49): OC::handleRequest()
#16 {main}

Maybe add that app enabled check to the isEnabled of the widget?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teams dashboard widget
5 participants