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

Create collab session #481

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

jonathanbcarlson
Copy link
Contributor

waiting to merge until able to join collab session with url

Copy link
Contributor

@timothycpoon timothycpoon left a comment

Choose a reason for hiding this comment

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

Hey Jonathan, all of this looks good. Just a couple things to fix, and then we need to wait on a collab-join link to merge. Thanks!

window.addEventListener("beforeunload", this.onLeave);
window.addEventListener("close", this.onLeave);
}
try {
let collabId = await sketch.constructCollabId(this.props.uid);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we create a collab session every time someone opens the editor, we will end up creating a lot of sessions that will never be used. I think it would be best to make creating a session an explicit action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I create a separate collab modal to create a collab session or should I put a create collab button under the Share modal?
image of create collab button in share modal

Copy link
Contributor

@timothycpoon timothycpoon Apr 21, 2021

Choose a reason for hiding this comment

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

I was thinking we could have a "Create Collab Session" button, which would then generate and display the URL. If you feel strongly either way you can do that though.

src/lib/fetch.js Outdated
* @param {string} id //id of CollabSession
*/

export const joinCollab = async (id) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that joining a session will be handled by the "collaborative mode" of the editor, so we probably don't need this function. However, if we do end up using this function, there are a couple issues: first, the currently planned endpoint would be collab/join/:id. Secondly, it should open a websocket, rather than make a GET request.

initiateCopy = () => {
navigator.clipboard.writeText(this.props.shareUrl).then(
initiateCopy = (textToBeCopied) => {
console.log("textToBeCopied is: ", textToBeCopied);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably remove debugging console.log

@timothycpoon
Copy link
Contributor

@jonathanbcarlson Hey, could you gate this feature behind developerAcc (as it makes a link to a non-existent feature) and merge it in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants