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

Add related resources panel component #3081

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

Pytal
Copy link
Contributor

@Pytal Pytal commented Aug 20, 2022

Add new component for nextcloud/server#28320

Screenshot (Shared items tab in Talk)

image

@ArtificialOwl since this component will be used in other apps how do we fetch the related resources of a Talk conversation, Deck board, Calendar event, et cetera?

@ArtificialOwl
Copy link

@ArtificialOwl since this component will be used in other apps how do we fetch the related resources of a Talk conversation, Deck board, Calendar event, et cetera?

It should be working the same as for Files.
The url of the OCS endpoint is : /related/{providerId}/{itemId}

replace providerId by "talk" or "deck".
replace itemId by the string or int Id that identify the current browsed item (room for talk, board for deck)

@Pytal Pytal force-pushed the feat/add-related-resources branch 8 times, most recently from 82e5d47 to a27989e Compare August 24, 2022 01:51
@Pytal Pytal added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 24, 2022
@Pytal Pytal force-pushed the feat/add-related-resources branch from a27989e to 5cc4640 Compare August 24, 2022 02:03
@Pytal
Copy link
Contributor Author

Pytal commented Aug 24, 2022

Keeping as draft for now

@PVince81
Copy link
Contributor

how about renaming from "related resources list" to just "resources list" ? it's just a name change and it already makes it more generic. having a component that is able to list resources can be useful in its own way (even if they are not "related")

@Pytal Pytal force-pushed the feat/add-related-resources branch from 5cc4640 to d297e01 Compare August 24, 2022 23:05
@Pytal
Copy link
Contributor Author

Pytal commented Aug 24, 2022

how about renaming from "related resources list" to just "resources list" ? it's just a name change and it already makes it more generic. having a component that is able to list resources can be useful in its own way (even if they are not "related")

There's no "related resources list" string do you mean renaming the component to NcResourceList?

@Pytal Pytal force-pushed the feat/add-related-resources branch from d297e01 to a028cd7 Compare August 25, 2022 00:24
@Pytal Pytal marked this pull request as ready for review August 25, 2022 00:27
@Pytal Pytal requested review from nickvergessen, raimund-schluessler and a team and removed request for a team August 25, 2022 00:27
@PVince81
Copy link
Contributor

how about renaming from "related resources list" to just "resources list" ? it's just a name change and it already makes it more generic. having a component that is able to list resources can be useful in its own way (even if they are not "related")

There's no "related resources list" string do you mean renaming the component to NcResourceList?

I meant that yes. But now going through the code I see that the logic is highly specific to that. I didn't think it would also contain the XHR requests for related resources and more logic. So the current code makes it very specific to related resources and is then reusable.

I still have mixed feelings for having this here, wondering if it shouldn't then be a different nextcloud repo and lib. On the other hand a specific lib for just 1-2 components feels like overkill. So perhaps we just move forward with this for now.

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

only one comment about accessible name
can also be resolved separately in case it becomes complicated

src/components/NcRelatedResourcesPanel/NcResource.vue Outdated Show resolved Hide resolved
Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal Pytal force-pushed the feat/add-related-resources branch from a028cd7 to fc75afd Compare August 26, 2022 00:58
@Pytal
Copy link
Contributor Author

Pytal commented Aug 26, 2022

I still have mixed feelings for having this here, wondering if it shouldn't then be a different nextcloud repo and lib. On the other hand a specific lib for just 1-2 components feels like overkill. So perhaps we just move forward with this for now.

Yeah this solution as well as the alternative would incur the benefits and drawbacks detailed in nextcloud-libraries/nextcloud-vue-dashboard#407

@juliushaertl
Copy link
Contributor

Note we branched off a stable6 branch and master will be the next major only compatible with Nextcloud 25+, so if needed please backport this pr also to stable6.

@PVince81
Copy link
Contributor

PVince81 commented Sep 1, 2022

related resources is planned for NC 25 only, so probably no backport needed

@juliushaertl juliushaertl merged commit 24ec9bc into master Sep 2, 2022
@juliushaertl juliushaertl deleted the feat/add-related-resources branch September 2, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants