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.

Sign up for GitHub

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

Jump to bottom

Extend dashboard api to allow listing of widgets #33658

Merged
merged 12 commits into from
Sep 16, 2022
Merged

Extend dashboard api to allow listing of widgets #33658

merged 12 commits into from
Sep 16, 2022

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Aug 23, 2022 •
edited
Loading

and allow filtering widget items by widget.

Extends on the api added by #26430

Apps will need to implement the new IIconWidget interface to have a icon url set.

Implementation for the "recommendations" widget can be found at: nextcloud/recommendations#543

@icewind1991 icewind1991 added the 2. developing Work in progress label Aug 23, 2022
@icewind1991 icewind1991 added this to the Nextcloud 25 milestone Aug 23, 2022
This was referenced Aug 23, 2022
Merged
Closed
@tobiasKaminsky
Copy link
Member

tobiasKaminsky commented Aug 24, 2022 •
edited
Loading

Thanks for this first version.
I tested it and it works so far!

Some questions/remarks

  • icon_class is not needed
  • icon_url must always be a real url
  • button name and url is missing (this is used e.g. at "recent activity" or "github notifications")
  • what is url?

@blizzz blizzz mentioned this pull request Aug 24, 2022
Merged
@icewind1991 icewind1991 mentioned this pull request Aug 29, 2022
Merged
@icewind1991
Copy link
Member Author

icewind1991 commented Aug 29, 2022 •
edited
Loading

icon_class is not needed

Icon url is left in so the web interface might use it in the future (and the data is there anyway)

button name and url

Extended to allow apps to register a button, example implementation added to the notes app

icon_url must always be a real url

This requires apps providing widgets to implement the new interface for getting the icon url, so the fallback exists for apps that don't.
Might be doable to have it fallback to the app icon instead so there is at least some icon.

what is url

Url to the apps' view of the widget, renamed it to widget_url to hopefully make it a bit more clear.

@blizzz blizzz mentioned this pull request Aug 30, 2022
Merged
This was referenced Aug 31, 2022
Closed
Closed
Closed
@PVince81
Copy link
Member

PVince81 commented Sep 1, 2022

can this be merged already or does it contain breaking changes ?

@icewind1991
Copy link
Member Author

can this be merged already or does it contain breaking changes ?

Api is still being tweaked

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 1, 2022
View reviewed changes
apps/dashboard/lib/Controller/DashboardApiController.php
'text' => $button->getText(),
'link' => $button->getLink(),
];
}, $widget->getWidgetButtons($this->userId)),

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 1 of OCP\Dashboard\IButtonWidget::getWidgetButtons cannot be null, possibly null value provided
@blizzz blizzz mentioned this pull request Sep 6, 2022
Merged
This was referenced Sep 6, 2022
Closed
Merged
@AndyScherzinger AndyScherzinger mentioned this pull request Sep 7, 2022
Closed
This was referenced Sep 7, 2022
Closed
Closed
This was referenced Sep 7, 2022
Merged
Merged
Merged
@julien-nc
Copy link
Member

@icewind1991 I hope you don't mind me adding commits here.

Discussion with @tobiasKaminsky started there #33940 (comment)
We were wondering how to deal with round/square item icons. We decided to extend the server api to allow defining a boolean (optionally, with another interface) to tell the clients if the item icons should be rendered round or not.

Then a new attribute (item_icons_round, false if the interface is not implemented) is passed in the /widgets endpoint.

I can make adjustments if needed or even remove this commit and add it in a PR to this branch. As you wish.

What do you think?

@blizzz blizzz mentioned this pull request Sep 9, 2022
Merged
@julien-nc julien-nc force-pushed the dashboard-api-widgets branch from 4c856fa to c448154 Compare September 12, 2022 09:49
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
icewind1991 and others added 9 commits September 15, 2022 18:05
@icewind1991
add dashboard api to list widgets
0e59447 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
allow filtering dashboard items api by widgets
7f52a99 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
allow adding button to dashboard api output
79adca6 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
change widget button api to support multiple button types
a3912e2 
Signed-off-by: Robin Appelman <robin@icewind.nl>
add IItemOptionWidget to define some item-related parameters, only ge…
845149b 
…tItemIconsRound() for now

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@icewind1991
move widget options into a Option class
d9e75f0 
Signed-off-by: Robin Appelman <robin@icewind.nl>
implement IAPIWidget in user_status
8e2e3b4 
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
implement IButtonWidget and IIconWidget in user_status
500ba95 
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@icewind1991
set round item icons for user status widget
9ba26ee 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@julien-nc julien-nc force-pushed the dashboard-api-widgets branch from bf3a6c6 to 9ba26ee Compare September 15, 2022 16:06
@julien-nc
Copy link
Member

Rebased on master.
The user_status stuff was merged here.
@tobiasKaminsky One last test since we have everything now (new interfaces and user_status implementing them) 🙏

@julien-nc julien-nc self-requested a review September 15, 2022 16:09
adjust empty array check
da74da8 
Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@tobiasKaminsky
Copy link
Member

Still works 🎉

@PVince81

This comment was marked as resolved.

Sign in to view
@nickvergessen
Add since tags
9c402eb 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@PVince81

This comment was marked as resolved.

Sign in to view
@nickvergessen
Fix phpunit
3009e02 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 16, 2022
@PVince81 PVince81 merged commit 703dd64 into master Sep 16, 2022
@PVince81 PVince81 deleted the dashboard-api-widgets branch September 16, 2022 10:18
@PVince81
Copy link
Member

it's merged!!! please update the PRs that were dependent on it :-)

@nickvergessen nickvergessen mentioned this pull request Sep 16, 2022
Merged
5 tasks
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Oct 10, 2022
@ChristophWurst
Copy link
Member

Will there be app dev docs?

@julien-nc
Copy link
Member

@ChristophWurst nextcloud/documentation#9211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 left review comments

@julien-nc julien-nc julien-nc approved these changes

@tobiasKaminsky tobiasKaminsky tobiasKaminsky approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees

@icewind1991 icewind1991

Labels
4. to release Ready to be released and/or waiting for tests to finish pending documentation This pull request needs an associated documentation update
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
7 participants
@icewind1991 @tobiasKaminsky @PVince81 @julien-nc @AndyScherzinger @ChristophWurst @nickvergessen