Implementing Inline commenting #1438 #1571
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@aspriya Good job! Is this ready ? You can make a research note at https://publiclab.org/post to showcase your Work. |
|
@jywarren and @ananyo2012, Following link is a video discription for the things I have implemented above. |
|
@ananyo2012, yes I think this is good to go and I have written a research note about this feature to introduce it. Thanks |
|
@icarito can we test this on the new |
|
Okay I missed this but let's push this to unstable and see how it goes. |
|
Whoa i didn't know you could switch branches in GitHub... so we'd open a new one to merge to master if this works? Thanks! |
|
And what is the link for unstable's instance? so we can try it out? |
|
Hm, next time maybe we should update stable from master first, because this PR now includes Aspriya's changes plus all previous PRs back to the last time stable was synced from master. That's OK -- i can open a new PR with the original intent? @aspriya -- you can still push to this feature branch, so that's fine. Can you think about writing functional tests for |
|
@icarito shall we open a new PR for the original => master merge, or can this one be switched back now that unstable is updated? |
|
@jywarren, sure I will start writing tests for those. Thanks 😃 |
|
Guys earlier today I went ahead and merged this into |
|
oh cool! But it's now 502... did it go down after a bit?
…On Fri, Aug 25, 2017 at 12:34 PM, Sebastian Silva ***@***.***> wrote:
Guys earlier today I went ahead and merged this into unstable branch from
the Github interface. A few minutes later, voilá:
http://unstable.publiclab.org/
I did not have to intervene.
The build log is at http://jenkins.laboratoriopublico.org/job/
Plots-Unstable/lastBuild/ including the migration.
Hope it's useful!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ61DagH7XkLx1L9pNe95o-WwHewQks5sbveEgaJpZM4OwcG0>
.
|
|
We're experimenting as you can see. I looked at the interface on Github
and I don't see an option because it's already resolved (merged). So I
guess a new PR is needed.
By the way the option to rebase the PR was hidden under the "EDIT" link
that is visible to change the PR title. I'll add a screenshot.
…On 25/08/17 09:18, Jeffrey Warren wrote:
@icarito <https://github.com/icarito> shall we open a new PR for the
original => master merge, or can this one be switched back now that
unstable is updated?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMMSyEdgXHF5Du3EMj2tvypfr8Fe4V5ks5sbte0gaJpZM4OwcG0>.
|
|
|
|
See the pull-down menu to pick the base under the editbox |
|
Whoa, I don't know, I'll check the logs. We should publish the logs for
staging maybe thru jenkins.
…On 25/08/17 12:11, Jeffrey Warren wrote:
oh cool! But it's now 502... did it go down after a bit?
On Fri, Aug 25, 2017 at 12:34 PM, Sebastian Silva
***@***.***>
wrote:
> Guys earlier today I went ahead and merged this into unstable branch
from
> the Github interface. A few minutes later, voilá:
> http://unstable.publiclab.org/
> I did not have to intervene.
> The build log is at http://jenkins.laboratoriopublico.org/job/
> Plots-Unstable/lastBuild/ including the migration.
> Hope it's useful!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1571 (comment)>,
> or mute the thread
>
<https://github.com/notifications/unsubscribe-auth/AABfJ61DagH7XkLx1L9pNe95o-WwHewQks5sbveEgaJpZM4OwcG0>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMMS0hhH6NNXLJG_lka8zWsR-gSL4hVks5sbwA1gaJpZM4OwcG0>.
|
|
And also change the 502 error with something useful such as the link to
the respective jenkins build queue.
…On 25/08/17 12:11, Jeffrey Warren wrote:
oh cool! But it's now 502... did it go down after a bit?
On Fri, Aug 25, 2017 at 12:34 PM, Sebastian Silva
***@***.***>
wrote:
> Guys earlier today I went ahead and merged this into unstable branch
from
> the Github interface. A few minutes later, voilá:
> http://unstable.publiclab.org/
> I did not have to intervene.
> The build log is at http://jenkins.laboratoriopublico.org/job/
> Plots-Unstable/lastBuild/ including the migration.
> Hope it's useful!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1571 (comment)>,
> or mute the thread
>
<https://github.com/notifications/unsubscribe-auth/AABfJ61DagH7XkLx1L9pNe95o-WwHewQks5sbveEgaJpZM4OwcG0>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMMS0hhH6NNXLJG_lka8zWsR-gSL4hVks5sbwA1gaJpZM4OwcG0>.
|
|
I don't know! there was nothing obvious in the logs. I've restarted it
manually. Let's monitor it.
…On 25/08/17 12:11, Jeffrey Warren wrote:
oh cool! But it's now 502... did it go down after a bit?
On Fri, Aug 25, 2017 at 12:34 PM, Sebastian Silva
***@***.***>
wrote:
> Guys earlier today I went ahead and merged this into unstable branch
from
> the Github interface. A few minutes later, voilá:
> http://unstable.publiclab.org/
> I did not have to intervene.
> The build log is at http://jenkins.laboratoriopublico.org/job/
> Plots-Unstable/lastBuild/ including the migration.
> Hope it's useful!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1571 (comment)>,
> or mute the thread
>
<https://github.com/notifications/unsubscribe-auth/AABfJ61DagH7XkLx1L9pNe95o-WwHewQks5sbveEgaJpZM4OwcG0>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMMS0hhH6NNXLJG_lka8zWsR-gSL4hVks5sbwA1gaJpZM4OwcG0>.
|
|
Great -- @aspriya, can you test this out at
http://unstable.publiclab.org/wiki/kits?raw=true ?
On Fri, Aug 25, 2017 at 1:39 PM, Sebastian Silva <notifications@github.com>
wrote:
… I don't know! there was nothing obvious in the logs. I've restarted it
manually. Let's monitor it.
On 25/08/17 12:11, Jeffrey Warren wrote:
> oh cool! But it's now 502... did it go down after a bit?
>
> On Fri, Aug 25, 2017 at 12:34 PM, Sebastian Silva
> ***@***.***>
> wrote:
>
> > Guys earlier today I went ahead and merged this into unstable branch
> from
> > the Github interface. A few minutes later, voilá:
> > http://unstable.publiclab.org/
> > I did not have to intervene.
> > The build log is at http://jenkins.laboratoriopublico.org/job/
> > Plots-Unstable/lastBuild/ including the migration.
> > Hope it's useful!
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <#1571 (comment)
>,
> > or mute the thread
> >
> <https://github.com/notifications/unsubscribe-auth/
AABfJ61DagH7XkLx1L9pNe95o-WwHewQks5sbveEgaJpZM4OwcG0>
> > .
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1571 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAMMS0hhH6NNXLJG_
lka8zWsR-gSL4hVks5sbwA1gaJpZM4OwcG0>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ8q__yf69vJ9HzIFHJLL8hhWW2V3ks5sbwbNgaJpZM4OwcG0>
.
|
|
remember you'll have to log in
On Fri, Aug 25, 2017 at 1:39 PM, Sebastian Silva <notifications@github.com>
wrote:
… I don't know! there was nothing obvious in the logs. I've restarted it
manually. Let's monitor it.
On 25/08/17 12:11, Jeffrey Warren wrote:
> oh cool! But it's now 502... did it go down after a bit?
>
> On Fri, Aug 25, 2017 at 12:34 PM, Sebastian Silva
> ***@***.***>
> wrote:
>
> > Guys earlier today I went ahead and merged this into unstable branch
> from
> > the Github interface. A few minutes later, voilá:
> > http://unstable.publiclab.org/
> > I did not have to intervene.
> > The build log is at http://jenkins.laboratoriopublico.org/job/
> > Plots-Unstable/lastBuild/ including the migration.
> > Hope it's useful!
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <#1571 (comment)
>,
> > or mute the thread
> >
> <https://github.com/notifications/unsubscribe-auth/
AABfJ61DagH7XkLx1L9pNe95o-WwHewQks5sbveEgaJpZM4OwcG0>
> > .
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1571 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAMMS0hhH6NNXLJG_
lka8zWsR-gSL4hVks5sbwA1gaJpZM4OwcG0>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ8q__yf69vJ9HzIFHJLL8hhWW2V3ks5sbwbNgaJpZM4OwcG0>
.
|
|
I was able to open an inline comment box, but nothing happened when I pressed publish. Should the button become "depressed" like in the regular comment box? Also, instead of the page title appearing in the inline comment box, maybe just "Comment"? Looking good though! |
|
And yes, @aspriya, please open a new PR - though it can be from the same branch! |
|
I tested this too. There must be some error in submission as I too don't see the comment being published when I publish the comment button. Also I see when I try to edit the line in inline-markdown-editor the line is not updated immediately. I see it updated only when I refresh the page. The Ajax style implementation is not working. |
|
Any updates, Ashan? Thanks!
…On Sat, Aug 26, 2017 at 5:40 PM, Ananyo Maiti ***@***.***> wrote:
I tested this too. There must be some error in submission as I too don't
see the comment being published when I publish the comment button. Also I
see when I try to edit the line in inline-markdown-editor the line is not
updated immediately. I see it updated only when I refresh the page. The
Ajax style implementation is not working.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ3GvzG2feRGgBROj7-aiScnL4AFIks5scJDBgaJpZM4OwcG0>
.
|
|
@jywarren and @ananyo, Yes there seems to be a problem. But If you wait
about one minute after pressing the `publish` button, the comment get
submitted. But in my local machine all works fine. may be it's slow because
this is staging server?
As @jywarren stated, yes having the title of the form only as `Comment`
seems enough
Following is a inline comment I added to staging server. I waited about 1
min after pressing the publish button.
[image](https://user-images.githubusercontent.com/13878973/29861863-653006e2-8d88-11e7-985d-44ca7419c3e1.png)
On Tue, Aug 29, 2017 at 10:50 PM, Jeffrey Warren <notifications@github.com>
wrote:
… Any updates, Ashan? Thanks!
On Sat, Aug 26, 2017 at 5:40 PM, Ananyo Maiti ***@***.***>
wrote:
> I tested this too. There must be some error in submission as I too don't
> see the comment being published when I publish the comment button. Also I
> see when I try to edit the line in inline-markdown-editor the line is not
> updated immediately. I see it updated only when I refresh the page. The
> Ajax style implementation is not working.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#1571 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-
auth/AABfJ3GvzG2feRGgBROj7-aiScnL4AFIks5scJDBgaJpZM4OwcG0>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANPGvUFh1LuozBFznIDbXQXtCpSeb_Iaks5sdEhKgaJpZM4OwcG0>
.
|
|
Hm, ok - (also you can't upload images by email but only on Github.com) @icarito is unstable really quite slow and might this be working but really slowly? Is there any way to improve responsiveness on unstable? I've noticed slowness myself. |
|
Just saw your message, I'll check it out. Perhaps we should expose app
logging from staging / unstable.
…On 29/08/17 15:57, Jeffrey Warren wrote:
Hm, ok - (also you can't upload images by email but only on Github.com)
@icarito <https://github.com/icarito> is unstable really quite slow
and might this be working but really slowly? Is there any way to
improve responsiveness on unstable? I've noticed slowness myself.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1571 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMMS4Ry4vUmbUGLkqX2tFgmPyPGS3xsks5sdHsdgaJpZM4OwcG0>.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!
rake test:allPlease be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software
We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays. Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.
Thanks!