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(contacts-menu): implement custom javascript hook action #49375

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Nov 19, 2024

Summary

Adds a new frontend-only contacts menu action registered via registerContactsMenuAction("foo-bar") from @nextcloud/vue.

Note: Please focus on the contact menu changes when reviewing this PR. The modal you see might change and is part of the Calendar app.

2024-11-27.14-35-29.mp4

Checklist

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Does it make sense to register this in the backend?

One the one side we already have link actions, but on the other link actions make sense for php-only apps and javascript actions would always need JS code.

Asking because all other APIs we introduced lately for similar things are purely frontend. E.g. file actions, views, filters.
Also older API is frontend only (e.g. user list rows, user actions etc).

@st3iny st3iny force-pushed the feat/contacts-menu/js-hook-action branch from a40d301 to a579d7c Compare November 25, 2024 16:56
@st3iny
Copy link
Member Author

st3iny commented Nov 25, 2024

Does it make sense to register this in the backend?

One the one side we already have link actions, but on the other link actions make sense for php-only apps and javascript actions would always need JS code.

Asking because all other APIs we introduced lately for similar things are purely frontend. E.g. file actions, views, filters. Also older API is frontend only (e.g. user list rows, user actions etc).

Hmm, that is actually a good point. But since I'm extending an existing API here, I'd prefer to keep it in the backend for now. Otherwise, it might get confusing as the avatar menu is populated from multiple sources of truth.

I'll think about it again.

@st3iny st3iny force-pushed the feat/contacts-menu/js-hook-action branch from a579d7c to b48320c Compare November 26, 2024 20:42
@st3iny
Copy link
Member Author

st3iny commented Nov 26, 2024

Ferdinand is right. The OCP API doesn't make sense. The menu has to registered from the frontend only, via @nextcloud/vue.

:key="idx"
:key="`${idx}-link`"
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated, but my LSP complained that it is illegal to have the same key on the on both nodes of v-if/v-else.

@st3iny st3iny marked this pull request as ready for review November 26, 2024 20:45
@st3iny st3iny added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 26, 2024
@st3iny st3iny added the blocked label Jan 6, 2025
@st3iny

This comment was marked as resolved.

@blizzz blizzz mentioned this pull request Jan 8, 2025
@st3iny st3iny force-pushed the feat/contacts-menu/js-hook-action branch from 279f0b9 to f914460 Compare January 9, 2025 13:44
@st3iny st3iny removed the blocked label Jan 9, 2025
@st3iny
Copy link
Member Author

st3iny commented Jan 9, 2025

This PR is now ready and not longer blocked.

:key="action.id"
:close-after-click="true"
class="other-actions"
@click="action.callback(contact)">
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick deluxe: consider handling async callbacks, to allow actions to signal when they are done. E.g. the screencast at nextcloud/calendar#6502 shows that there is a brief moment of no visual feedback before the modal opens. During that time the action button could switch to a loading state. On a technical level the callback would return a promise, and here you would go into loading state as long as the promise hasn't resolved.

This is not a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. This would be a good follow-up.

@ChristophWurst ChristophWurst removed the 3. to review Waiting for reviews label Jan 10, 2025
@ChristophWurst ChristophWurst added the 4. to release Ready to be released and/or waiting for tests to finish label Jan 10, 2025
@st3iny st3iny merged commit 376b00c into master Jan 10, 2025
120 checks passed
@st3iny st3iny deleted the feat/contacts-menu/js-hook-action branch January 10, 2025 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: contacts menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants