Skip to content

Conversation

@DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Oct 30, 2023

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
image image
image image
image image

🚧 Tasks

  • Code review
  • Visual review

🏁 Checklist

@nickvergessen
Copy link
Member

Ah, I missed this is still open. I bumped dialogs in #10821 So make sure to rebase and retest :)

@DorraJaouad
Copy link
Contributor Author

Before I move further, we should migrate the duplicate session dialog to NcDialog too, right?

https://github.com/nextcloud/spreed/blob/main/src/store/participantsStore.js#L790-L825

It will be replaced by an event emission from the store and listened by the main components calling 'joinConversation' :

App.vue
FilesSidebarTabApp.vue
PublicShareAuthSidebar.vue
PublicShareSidebar.vue

For the others, like LeftSidebar's function call, we can use the NcDialog in App component instead

@nickvergessen
Copy link
Member

Lets do that in a different PR. It's a more problematic thing to check and test than the currently handled deletions.

@DorraJaouad DorraJaouad force-pushed the fix/noid/update-to-ncdialog branch from f590f29 to eb843f1 Compare November 3, 2023 07:44
@DorraJaouad DorraJaouad marked this pull request as ready for review November 3, 2023 07:53
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.

I would use error in all cases like you used already for the breakout room deletion

@DorraJaouad DorraJaouad force-pushed the fix/noid/update-to-ncdialog branch from eb843f1 to 3e7a41c Compare November 8, 2023 13:14
@DorraJaouad DorraJaouad merged commit dc49879 into main Nov 8, 2023
@DorraJaouad DorraJaouad deleted the fix/noid/update-to-ncdialog branch November 8, 2023 13:41
ShGKme added a commit to nextcloud-libraries/nextcloud-vue that referenced this pull request Nov 10, 2023
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.

3 participants