-
Notifications
You must be signed in to change notification settings - Fork 8
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
[MMB-219] Update docs #113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @surminus, this is a great start! I've requested some changes but I think it'll be easier for us to pair on this for a bit as there have been lots of changes to the public API, some potentially confusing to understand as there's not much context documented in the DR. If that works with you, I'll book something in the calendar.
docs/usage.md
Outdated
|
||
Emitted when members enter, leave and their location is updated. Called with an array of all the members in the space. | ||
Emitted when a member profile updates a space. Called with the member updating the space. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emitted when a member profile updates a space. Called with the member updating the space. | |
Emitted when members enter, leave and their location is updated. Called with the member updating the space. |
27384dc
to
6da38d5
Compare
Thanks @Srushtika, I've attempted to go through and make this a little more consistent and up to date. Let me know if it makes sense (and make sure I haven't nerfed it during a rebase). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments on a few things that I think need clarification. Feel free to shout any questions at me.
docs/class-definitions.md
Outdated
``` | ||
The argument supplied to the callback is a [SpaceMember](#spacemember) object representing the member entering the space. | ||
|
||
- #### **leave** | ||
|
||
Listen to leave events of members. Note that the leave event will only fire once the [offlineTimeout](#spaceoptions) has passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is now untrue. As far as I understand it, leave
is fired as soon as a member calls space.leave()
or is disconnected. A remove
event is then fired after the offlineTimeout
period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's my understanding too. Can you please check this in the implementation @surminus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please check this in the implementation
Seems to be but honestly not entirely confident that I understand the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @surminus , we are almost there! Requested a few more changes.
Thank you @m-hulbert and @Srushtika, I've attempted another pass and hopefully it's almost there... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a few comments related to the asyncifying I did in #116 (sorry about that 😬 )
@@ -189,16 +247,15 @@ space.enter({ | |||
avatar: 'https://slides-internal.com/users/clemons.png', | |||
}); | |||
|
|||
// Listen to events published on "mousemove" by all members | |||
space.cursors.subscribe('update', (cursorUpdate) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only works with space.cursors.subscribe("cursorsUpdate", ...
& not as space.cursors.subscribe("update",...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will update this PR... I think this should change to be update
to be consistent, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmars @surminus looks like it's a bug as it's something we agreed on. It's also possible that it slipped the implementation part as there were quite a few changes - are you able to check this please?https://ably.atlassian.net/wiki/spaces/product/pages/2659713025/MMDR14+Spaces+SDK+New+API+Design#Member-cursors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
74c8a53
to
3990730
Compare
@Srushtika I've squashed all my fixup commits and hopefully resolved the comments that were left (except the one from Arti about the incorrect API, which I've raised another PR for)... if you want to push some other changes to the branch feel free. |
458a6f3
to
f653b47
Compare
I've tried to organise this to include the members namespace, and tried to keep the examples consistent.
f653b47
to
6934f5f
Compare
Update docs to use the latest API.