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

feat(comments): Plug in comments into activity sidebar tab if available #41491

Merged
merged 4 commits into from
Nov 16, 2023
Merged

feat(comments): Plug in comments into activity sidebar tab if available #41491

merged 4 commits into from
Nov 16, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Nov 15, 2023 •
edited
Loading

Summary

This is a more stable approach than #41483, if activity is enabled we plug into their stream and provide the comments.

Screenshot

vokoscreenNG-2023-11-15_12-22-39.mp4

Checklist

@susnux susnux added this to the Nextcloud 28 milestone Nov 15, 2023
@susnux susnux mentioned this pull request Nov 15, 2023
Merged
@susnux susnux requested review from artonge, a team, sorbaugh and emoral435 and removed request for a team November 15, 2023 11:31
@susnux susnux force-pushed the fix/merge-comments-activity-v2 branch from e02fbe1 to 0aa2c7e Compare November 15, 2023 12:17
artonge
artonge approved these changes Nov 15, 2023
View reviewed changes
apps/comments/lib/Activity/Provider.php Outdated Show resolved Hide resolved
@susnux susnux mentioned this pull request Nov 15, 2023
Closed
4 tasks
nickvergessen
nickvergessen requested changes Nov 15, 2023
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

This breaks the comments API when used by other apps again, as pointed out in the previous PR

@susnux
Copy link
Contributor Author

susnux commented Nov 15, 2023

This breaks the comments API when used by other apps again, as pointed out in the previous PR

Did you test this? This PR should only change the sidebar for the files app

@nickvergessen
Copy link
Member

Did you test this?

yes, something is hardcoding the object type to files and try tomorrow again (or you install https://github.com/nextcloud/announcementcenter and try it yourself)

@susnux
Copy link
Contributor Author

susnux commented Nov 15, 2023

OK I know whats going on here... Have I ever said I hate mixins?
I had some weird issues were the type was not set, so I set it to files, well turns out the comments API sets it using mixins syntax from hell. I will fix this.

@susnux susnux disabled auto-merge November 15, 2023 22:47
@susnux susnux force-pushed the fix/merge-comments-activity-v2 branch from 0aa2c7e to 858d3f7 Compare November 15, 2023 23:11
@susnux
Copy link
Contributor Author

susnux commented Nov 15, 2023

Should be fixed now @nickvergessen

I really do not like code that hides what happens, in this case the comment type was injected only in one of the entry points as as a Mixin (**!**) instead of declaring it a prop so you know that it is there are need to be set / can be set.

@blizzz blizzz mentioned this pull request Nov 16, 2023
Merged
@artonge artonge force-pushed the fix/merge-comments-activity-v2 branch from 858d3f7 to 74f127d Compare November 16, 2023 10:33
@artonge
Copy link
Contributor

artonge commented Nov 16, 2023

Rebased and fixed come's comment

nickvergessen
nickvergessen requested changes Nov 16, 2023
View reviewed changes
apps/comments/src/mixins/CommentView.ts Outdated Show resolved Hide resolved
apps/comments/src/mixins/CommentView.ts Show resolved Hide resolved
apps/comments/src/services/CommentsInstance.js Show resolved Hide resolved
apps/comments/src/views/Comments.vue Outdated Show resolved Hide resolved
@susnux susnux requested a review from come-nc November 16, 2023 10:53
@artonge artonge force-pushed the fix/merge-comments-activity-v2 branch from 74f127d to fa1e09b Compare November 16, 2023 10:57
@artonge
Copy link
Contributor

artonge commented Nov 16, 2023 •
edited
Loading

@nickvergessen I tested with announcement center. Should be fine now. :)

@susnux susnux force-pushed the fix/merge-comments-activity-v2 branch from fa1e09b to b032929 Compare November 16, 2023 10:59
@susnux
Copy link
Contributor Author

susnux commented Nov 16, 2023 •
edited
Loading

@nickvergessen resolved you comments should be fine now

Edit

I tested with announcement center. Should be fine now. :)

Oh we did this simultaneously :/

@nickvergessen
Copy link
Member

Oh we did this simultaneously :/

All hail to git push --force-with-lease I guess

@nickvergessen
Copy link
Member

Now the hardcoded files is back?
/remote.php/dav/comments/files/1

@nickvergessen
Copy link
Member

Now the hardcoded files is back?

Only on posting, loading old comments works properly

@susnux

This comment was marked as resolved.

Sign in to view
@nickvergessen
Copy link
Member

No resource type is set, what ressource type do you expect it to use?

The first argument is the resource type!?

@susnux
fix(comments): Provide resourceType as property instead of mixin
9244d53 
Also fix typos where `ressource` instead of `resource` was used.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/merge-comments-activity-v2 branch from b032929 to f94e6e2 Compare November 16, 2023 12:22
@susnux susnux force-pushed the fix/merge-comments-activity-v2 branch from f94e6e2 to aed3bd6 Compare November 16, 2023 12:29
susnux and others added 2 commits November 16, 2023 14:16
@susnux @nickvergessen
chore: Compile assets
75284bc 
Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Louis Chemineau <[email protected]>
@nickvergessen
Fix property name
6eb182a 
Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen
Copy link
Member

There were some mixing of the property name making it fallback to the default 'files'
I added a commit (and merged the recompile into the previous commit)

But now it works in files and announcements.

@susnux
Copy link
Contributor Author

susnux commented Nov 16, 2023

Cypress is unrelated (flaky user-creation test)

@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 16, 2023
@susnux susnux merged commit 84004d0 into master Nov 16, 2023
51 checks passed
@susnux susnux deleted the fix/merge-comments-activity-v2 branch November 16, 2023 17:25
@nickvergessen nickvergessen mentioned this pull request Dec 13, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artonge artonge artonge approved these changes

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

@nickvergessen nickvergessen nickvergessen approved these changes

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh was automatically assigned from nextcloud/server-frontend

@emoral435 emoral435 Awaiting requested review from emoral435 emoral435 was automatically assigned from nextcloud/server-frontend

@come-nc come-nc Awaiting requested review from come-nc

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: comments ❄️ 2023-Winter
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

📑 Sidebar: combine file Activity and Comments into »Activity« timeline tab (possibly also "Versions")
5 participants
@susnux @nickvergessen @artonge @AndyScherzinger @come-nc