-
Notifications
You must be signed in to change notification settings - Fork 208
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
Synchronous editing documentation #933
base: synch-editing-tests
Are you sure you want to change the base?
Synchronous editing documentation #933
Conversation
Added some documentation related to the new synchronous editing feature we are introducing. @jywarren @jainaman224 |
This is great. Can you actually merge this commit into the main synchronous
editing branch so they get squashed together? This'll be nice as it'll all
land in one commit in the main branch. Thanks!
…On Tue, Aug 13, 2019 at 5:00 PM Vidit ***@***.***> wrote:
Added some documentation related to the new synchronous editing feature we
are introducing. @jywarren <https://github.com/jywarren> @jainaman224
<https://github.com/jainaman224>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#933?email_source=notifications&email_token=AAAF6J3O3QLFNVREPQLJOL3QEMOD3A5CNFSM4ILDPIBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4G66UI#issuecomment-521006929>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J2AZ75AYEWC4WW5H3DQEMOD3ANCNFSM4ILDPIBA>
.
|
1. The _action-cable-testing_ gem is used for the feature's testing. It has some really | ||
cool testing functionality which was required for our use case. | ||
|
||
2. Currently we have separate tests written for connection related features and channel |
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.
Documentation should not have words like currently
as in future, it will give false negative impressions.
Try to read the document from the view of newbie of MK after 2 years of publishing this doc. Documents are least maintained so we need to be extra careful.
|
||
2. The broadcasted data is finally caught by the _received_ function of _app/javascripts/channels/concurrent_editing.js_ | ||
|
||
3. Finally the _received_ function calls the _synchronizeData_ function to 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.
Similary finally
should be changed or removed
@@ -15,6 +15,10 @@ App.concurrent_editing = App.cable.subscriptions.create("ConcurrentEditingChanne | |||
}, | |||
|
|||
speak: function(changes) { | |||
/* Called when an image is updated from Map.js ('saveImage' function). | |||
* This function calls concurrent_editing_channel.rb's 'sync' method |
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.
Please don't write This
.
Reason a new developer writes comments between 18 and 19 LOC then this will become outdated and ambiguous.
@@ -388,6 +388,9 @@ MapKnitter.Map = MapKnitter.Class.extend({ | |||
if (this.editing._mode !== "lock") { e.stopPropagation(); } | |||
}, | |||
|
|||
/* Called by the concurrent_editing.js channel's 'received' function (app/assets/javascripts/channels/concurrent_editing.js). | |||
* It recieves a list of updated warpables,i.e. list of images with updated corner points. The aim of writing this function |
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.
Write name of function instead of pronouns. Change `It'
@@ -275,6 +275,7 @@ def add_tag(tagname, user) | |||
end | |||
|
|||
def fetch_map_data | |||
# fetches a list of updated warpables along with their corners in a json format. | |||
data = warpables |
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.
Uppercase F
in fetches
I have added a few minor changes to the documentation. It is awesome and perfect already. Just we require to be careful about future prs so written some comments. Please address and squash them in other pr. Close this pr afterwards. |
part of #804