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

Add sharing deck card with a conversation #5110

Merged
merged 3 commits into from
Feb 15, 2021
Merged

Add sharing deck card with a conversation #5110

merged 3 commits into from
Feb 15, 2021

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Feb 9, 2021 •
edited
Loading

Added deck action for sharing/linking that uses the RoomSelector.
Improved RoomSelector

Fixes #5106
Requires nextcloud/deck#2772 (+ info.xml max-version hack) for testing.

Issues for later:

Open topics:

  • is the registration for the script in the right place or do we want another special event-thingy for deck to tell us when to register ? (like OCA\Files\Event\LoadSidebar)

@PVince81 PVince81 added this to the 💖 Next Major (22) milestone Feb 9, 2021
@PVince81 PVince81 self-assigned this Feb 9, 2021
@PVince81 PVince81 mentioned this pull request Feb 9, 2021
Closed
5 tasks
@nickvergessen
Copy link
Member

check for lobby state + moderator status to see if postable while filtering the list ?

Wouldn't care too much. Posting is blocked in the end, just show an error.

nickvergessen
nickvergessen reviewed Feb 10, 2021
View reviewed changes
src/deck.js Outdated Show resolved Hide resolved
nickvergessen
nickvergessen reviewed Feb 10, 2021
View reviewed changes
src/deck.js Outdated Show resolved Hide resolved
nickvergessen
nickvergessen reviewed Feb 10, 2021
View reviewed changes
src/deck.js Outdated Show resolved Hide resolved
nickvergessen
nickvergessen reviewed Feb 10, 2021
View reviewed changes
src/deck.js Outdated Show resolved Hide resolved
@nickvergessen
Copy link
Member

Looks good apart from the wordings

@PVince81 PVince81 mentioned this pull request Feb 10, 2021
Closed
@PVince81
Copy link
Member Author

wait, the message service is not done yet

@PVince81 PVince81 force-pushed the enh/5106/share-deck-card-with-conversation branch from d616d6b to 685ef1d Compare February 10, 2021 15:30
@PVince81
Copy link
Member Author

I've squashed our common changes + messagesService changes.

nickvergessen
nickvergessen previously approved these changes Feb 10, 2021
View reviewed changes
juliushaertl
juliushaertl reviewed Feb 10, 2021
View reviewed changes
src/deck.js Outdated Show resolved Hide resolved
@PVince81 PVince81 force-pushed the enh/5106/share-deck-card-with-conversation branch from 685ef1d to ef2d90d Compare February 10, 2021 18:15
@PVince81
Copy link
Member Author

PVince81 commented Feb 10, 2021 •
edited
Loading

comments addressed, adjusted, retested and squashed

@PVince81 PVince81 dismissed nickvergessen’s stale review February 11, 2021 08:46

a change of mind regarding the way the dialog is invoked

@PVince81 PVince81 force-pushed the enh/5106/share-deck-card-with-conversation branch from ef2d90d to 60e02a7 Compare February 11, 2021 09:21
@PVince81
Copy link
Member Author

@nickvergessen see last commit, I've moved away from OCA.Collaboration and also made the dialog configurable.

Please review.

@PVince81 PVince81 force-pushed the enh/5106/share-deck-card-with-conversation branch from 60e02a7 to f2655ff Compare February 12, 2021 09:01
@PVince81
Copy link
Member Author

Rebased and added commit for filtering out the current room.

To expose the current room I expose the full vue instance in OC.Talk.instance which I believe can be more useful in the future for components running on that page. Let me know if this is ok or if this is too much.

@PVince81 PVince81 force-pushed the enh/5106/share-deck-card-with-conversation branch from f2655ff to c2c1fc1 Compare February 12, 2021 09:03
@PVince81
Copy link
Member Author

PVince81 commented Feb 12, 2021 •
edited
Loading

squashed the "wrong file" deletion, please re-review

@nickvergessen
Copy link
Member

Apps live in OCA. 🙈

Also we are there already, so reuse (with same precausions)

spreed/src/utils/webrtc/webrtc.js

Lines 511 to 514 in a542d70

if (!window.OCA.Talk) {
window.OCA.Talk = {}
}
window.OCA.Talk.SimpleWebRTC = webrtc

@PVince81
Copy link
Member Author

Apps live in OCA.

I did use OCA.Talk here: c2c1fc1#diff-27653c212e1cfe533e4eb2f7d0d3f89604c9de48a09583b4cbbbcbd08a07da79R160

I'll add a namespace check, just in case

@PVince81 PVince81 force-pushed the enh/5106/share-deck-card-with-conversation branch from c2c1fc1 to 77d31f4 Compare February 12, 2021 11:03
@PVince81
Copy link
Member Author

extra check squashed into last commit

nickvergessen
nickvergessen approved these changes Feb 12, 2021
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.

Ready to merge, but I guess we wait for nextcloud/deck#2772

PVince81 and others added 3 commits February 12, 2021 13:56
@PVince81 @nickvergessen
Add sharing deck card with a conversation
ddf06db 
Added deck action for sharing/linking that uses the RoomSelector.
Improved RoomSelector

Co-authored-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Don't use collaboration endpoint to trigger RoomSelector
fa55c6c 
Deck plugin now creates its own Vue VM to instantiate the
RoomSelector.
RoomSelector now allows to specify properties for dialog title and
whether the result list should be filtered.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Remove current room from RoomSelector
ab45b79 
Expose the Talk Vue instance on OCA.Talk.instance.
In RoomSelector, grab the current room from that instance to filter it
out, when applicableRemove current room from RoomSelector

Expose the Talk Vue instance on OCA.Talk.instance.
In RoomSelector, grab the current room from that instance to filter it
out, when applicable.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Member Author

rebased for CI

@PVince81 PVince81 force-pushed the enh/5106/share-deck-card-with-conversation branch from 77d31f4 to ab45b79 Compare February 12, 2021 12:56
@PVince81
Copy link
Member Author

nextcloud/deck#2772 was merged, merging as well...

is this something to backport ?

@PVince81 PVince81 merged commit 6f88aa9 into master Feb 15, 2021
@PVince81 PVince81 deleted the enh/5106/share-deck-card-with-conversation branch February 15, 2021 09:03
@nickvergessen
Copy link
Member

/backport to stable21

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Feb 15, 2021
Closed
@nickvergessen
Copy link
Member

/backport to stable21.1

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Feb 19, 2021
Merged
@julien-nc julien-nc mentioned this pull request Aug 26, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

Assignees

@PVince81 PVince81

Labels
4. to release enhancement
Projects
None yet
Milestone
v12.0.0-rc.1
Development

Successfully merging this pull request may close these issues.

Register Deck action for sharing Deck with a conversation
3 participants
@PVince81 @nickvergessen @juliushaertl