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

Wiki Inline-Commenting #1438 #1593

Closed

Conversation

aspriya
Copy link
Collaborator

@aspriya aspriya commented Aug 30, 2017

Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!

  • all tests pass -- rake test:all
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request is descriptively named with #number reference back to original issue

Picking up from where #1571 left off.

@aspriya aspriya force-pushed the add_a_reference_collumn_for_comments branch 2 times, most recently from dd4a2b2 to 7450e1b Compare August 30, 2017 14:01
@jywarren
Copy link
Member

jywarren commented Aug 30, 2017 via email

@aspriya aspriya force-pushed the add_a_reference_collumn_for_comments branch from 7450e1b to cbfff1d Compare August 30, 2017 16:26
@PublicLabBot
Copy link

PublicLabBot commented Aug 30, 2017

1 Warning
⚠️ New migrations added. Please update schema.rb.example by overwriting it with a copy of the up-to-date db/schema.rb.
2 Messages
📖 @aspriya Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution!
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 🚫 Danger

@jywarren
Copy link
Member

Ooh, cool - but see the warning about new schema file!

@jywarren
Copy link
Member

jywarren commented Aug 31, 2017

I wanted to note that I think /all/ rich wiki updating is a bit slow. We'll look into this, but I believe we should:

  • monitor unstable for performance (@icarito)
  • add a "spinner" icon as with the Post button on /post (this also happens in regular comments, i think) so people know it's doing something
  • fix the schema file as advised by Dangerbot above

then let's test one more time -- i'd like to work a bit more on how the comments are displayed too; perhaps we can show 3 comments by the comment icons, instead of the full display shown in your screenshot:

image

Then that full display could be toggled to show.

@@ -15,7 +14,8 @@ function setupWiki(node_id, title, raw, logged_in) {
preProcessor: preProcessMarkdown,
postProcessor: postProcessContent,
extraButtons: {
"fa-question": questionForm
"fa-question": questionForm,
"fa-comment": setupCommentFunction
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this:

<% if params[:inlineCommenting] %>"fa-comment": setupCommentFunction<% end %>

? That way we can test this more on production without committing to it just yet. And we can publish the general Rich Wikis inline features without also having to publish inline commenting which may take a little longer?

Copy link
Member

Choose a reason for hiding this comment

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

Hi, @aspriya - i think if you can add this flag we can merge and see how this is working. Thanks!

@@ -49,6 +49,7 @@ ActiveRecord::Schema.define(:version => 20170824172336) do
t.string "mail", :limit => 64
t.string "homepage"
t.integer "aid", :default => 0, :null => false
t.string "reference"
Copy link
Member

Choose a reason for hiding this comment

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

Here I believe you have to update the timestamp at the top of the file to match the latest migration version -- that may be what's stopping you!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh thanx. Corrected this

@icarito
Copy link
Member

icarito commented Aug 31, 2017 via email

@aspriya aspriya force-pushed the add_a_reference_collumn_for_comments branch from cbfff1d to 443108d Compare September 1, 2017 13:57
@jywarren
Copy link
Member

jywarren commented Sep 8, 2017

I made the "hidden" flag in javascript so this will only work when this is at the end of the URL: ?inlineComments=true

Now all that's left is:

  • add a "spinner" icon as with the Post button on /post (this also happens in regular comments, i think) so people know it's doing something

Very close!

@jywarren
Copy link
Member

Hi, @aspriya - what do you think about these changes? Hope you're well!

@ryzokuken
Copy link
Member

@jywarren @aspriya hasn't replied to this thread for a very long time. Is this mergeable, or should we close this for now?

@jywarren
Copy link
Member

jywarren commented Jan 26, 2018 via email

@ryzokuken
Copy link
Member

@jywarren I could definitely give this a try!

@SidharthBansal
Copy link
Member

As the person is inactive for more than a month, I am closing the PR. In case you want to push changes please feel free to open a new PR OR reopen this PR and add additional changes to it.
Thanks for contributing on Public Lab

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.

None yet

6 participants