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

Leave comments on someone elses pages #47 #261

Merged
merged 46 commits into from Aug 1, 2017

Conversation

Projects
None yet
3 participants
@Abijeet
Member

Abijeet commented Jan 1, 2017

These page contains details for the implementation of the comments section for BookStack. Issue is here.

Functionality to implement

  • Add a simple editor with Markdown support.
  • Ability to add comments.
  • Ability to edit own comments.
  • Ability to delete your own comments
  • Ability to link directly to a comment in the page
  • Ability to reply to a comment (upto 2 levels?)
  • Display total comments on the page summary maybe.
  • User should not be able to edit other's comments.

Future enhancements

  • Comment change history
  • Add roles to comment.
  • Inline document comments where users can highlight sections of pages and comment on tThis is sparta!hose.
  • Like comments
  • Highlight people via your comments with a "@Abijeet", similar to Facebook.

Database schema

The database schema will be a simple tree based schema.

Reviewed various tree based schema's structure for SQL described here http://mikehillyer.com/articles/managing-hierarchical-data-in-mysql/

After analyzing the various schema structures, I've finally decided to go ahead with the Adjacency List based on its simplicity. Following will be the schema structure -

data-model

Task list

Breaking down the whole thing into smaller tasks,

  • Create the migration script.
    • Table name is comments.
  • Create the comment entity (extends Ownable).
  • Create the comment repo.
  • Create the permissions for comments, which will be the same as attachments.
  • Create the comments controller with the following methods,
    • add
    • update
    • delete
    • getLastXComments
    • getChildComments
  • Create the views
    • Create a directive with simple markdown editor to save markdown based comments.
    • Replying to a comment.
    • Editing a comment.
  • Permissions
  • Allow direct linking to a comment
  • Implement translations
  • Manual tests
  • Automated tests
  • Running load tests
  • Setting up the demo data.
  • Code cleanup
  • Code review by @ssddanbrown
  • Merge with master
  • Update documentation

@ssddanbrown - Please do not merge this, I'll let you know when this is ready to be merged.

@Abijeet Abijeet changed the title from Adding the comments functionality. to Leave comments on someone elses pages #47 Jan 1, 2017

Abijeet added some commits Jan 3, 2017

@Abijeet

This comment has been minimized.

Show comment
Hide comment
@Abijeet

Abijeet Jan 14, 2017

Member

User Interface

Page load

When the page loads, we'll make an AJAX call to load the 30 most recent parent comments.

Fetching the child comments

On page load the child comments will not be fetched, click on Show replies at the bottom of the parent comment will make another AJAX call to load the replies, which will also be loaded 30 at a time.

Only single levels of depth are allowed in the commenting architecture.

Deleting

A enabled delete icon will be presented to the user to delete the comment. A user can only delete comments if he is the owner or an administrator. Deleting will not wipe out the entire comment, but will simply empty the content in the database.

Editing

Editing will transform the comment being editing into a simple markdown editor that can then be used to update the comment.

Replying

Replying to a comment will open a editor right below the comment to which you are replying.

Adding

A new thread can be started by adding a comment to a editor at the bottom of the comment section.

Member

Abijeet commented Jan 14, 2017

User Interface

Page load

When the page loads, we'll make an AJAX call to load the 30 most recent parent comments.

Fetching the child comments

On page load the child comments will not be fetched, click on Show replies at the bottom of the parent comment will make another AJAX call to load the replies, which will also be loaded 30 at a time.

Only single levels of depth are allowed in the commenting architecture.

Deleting

A enabled delete icon will be presented to the user to delete the comment. A user can only delete comments if he is the owner or an administrator. Deleting will not wipe out the entire comment, but will simply empty the content in the database.

Editing

Editing will transform the comment being editing into a simple markdown editor that can then be used to update the comment.

Replying

Replying to a comment will open a editor right below the comment to which you are replying.

Adding

A new thread can be started by adding a comment to a editor at the bottom of the comment section.

@Abijeet Abijeet changed the title from Leave comments on someone elses pages #47 to [WIP] Leave comments on someone elses pages #47 Apr 2, 2017

@Abijeet

This comment has been minimized.

Show comment
Hide comment
@Abijeet

Abijeet Apr 25, 2017

Member

@ssddanbrown - Sorry for the lack of update on this. I've started working on this again recently.

For the comment box, I'm using https://simplemde.com/

Its a basic markdown editor, that looks and functions a lot like the Github markdown editor. Its MIT Licensed

Do you have any concerns? Would you like me to use a different editor?

Member

Abijeet commented Apr 25, 2017

@ssddanbrown - Sorry for the lack of update on this. I've started working on this again recently.

For the comment box, I'm using https://simplemde.com/

Its a basic markdown editor, that looks and functions a lot like the Github markdown editor. Its MIT Licensed

Do you have any concerns? Would you like me to use a different editor?

@ssddanbrown

This comment has been minimized.

Show comment
Hide comment
@ssddanbrown

ssddanbrown Apr 27, 2017

Member

Hi @Abijeet, Thanks for your continued work on this.

My concerns with SimpleMDE are that it's fairly chunky in the amount of code it brings in for a fairly simple use case such as this. I was thinking that a normal old textarea would be fine for something like this, At least initially until we get a feel for how it's used. Keeping the input simple prevents people getting to fancy in the comments and keeps them secondary to page content.

Let me know if you disagree with this though or if there's any technical reasons a more featureful editor would be better.

Member

ssddanbrown commented Apr 27, 2017

Hi @Abijeet, Thanks for your continued work on this.

My concerns with SimpleMDE are that it's fairly chunky in the amount of code it brings in for a fairly simple use case such as this. I was thinking that a normal old textarea would be fine for something like this, At least initially until we get a feel for how it's used. Keeping the input simple prevents people getting to fancy in the comments and keeps them secondary to page content.

Let me know if you disagree with this though or if there's any technical reasons a more featureful editor would be better.

@Abijeet

This comment has been minimized.

Show comment
Hide comment
@Abijeet

Abijeet Apr 28, 2017

Member
Member

Abijeet commented Apr 28, 2017

@@ -161,6 +161,8 @@ let setupPageShow = window.setupPageShow = function (pageId) {
}
});
// in order to call from other places.
window.setupPageShow.goToText = goToText;

This comment has been minimized.

@Abijeet

Abijeet Jun 10, 2017

Member

This has been done because I need to use it to highlight directly linked comments.

@Abijeet

Abijeet Jun 10, 2017

Member

This has been done because I need to use it to highlight directly linked comments.

/**
* Comments
*/
'comment' => 'Comentario',

This comment has been minimized.

@Abijeet

Abijeet Jun 10, 2017

Member

Used Google translate to translate this. Don't think all these languages are properly translated.

@Abijeet

Abijeet Jun 10, 2017

Member

Used Google translate to translate this. Don't think all these languages are properly translated.

@@ -117,6 +117,19 @@
<label>@include('settings/roles/checkbox', ['permission' => 'attachment-delete-all']) {{ trans('settings.role_all') }}</label>
</td>
</tr>
<tr>
<td>{{ trans('entities.comments') }}</td>

This comment has been minimized.

@Abijeet

Abijeet Jun 10, 2017

Member

permission settings for comments - similar to attachments / images

@Abijeet

Abijeet Jun 10, 2017

Member

permission settings for comments - similar to attachments / images

@Abijeet Abijeet closed this Jun 12, 2017

@Abijeet Abijeet reopened this Jun 12, 2017

<div class="row">
<div class="col-md-9 print-full-width">
<div class="page-content">
<div class="page-content" ng-non-bindable>

This comment has been minimized.

@Abijeet

Abijeet Jun 12, 2017

Member

Comments have to be put inside the page-show container, hence moved the ng-non-bindable to a stricter scope.

@Abijeet

Abijeet Jun 12, 2017

Member

Comments have to be put inside the page-show container, hence moved the ng-non-bindable to a stricter scope.

@Abijeet

This comment has been minimized.

Show comment
Hide comment
@Abijeet

Abijeet Jun 12, 2017

Member

@ssddanbrown - Can you please review the code and merge it when you get a chance?

I've completed everything mentioned in the checklist above other than the test data generator. I'll need some help with that as I'm unable to find that code. Basically I'd like to add comments on the demo website.

After the merge I'll also update any documentation that needs updating.

Member

Abijeet commented Jun 12, 2017

@ssddanbrown - Can you please review the code and merge it when you get a chance?

I've completed everything mentioned in the checklist above other than the test data generator. I'll need some help with that as I'm unable to find that code. Basically I'd like to add comments on the demo website.

After the merge I'll also update any documentation that needs updating.

@Abijeet Abijeet changed the title from [WIP] Leave comments on someone elses pages #47 to Leave comments on someone elses pages #47 Jun 12, 2017

@ssddanbrown ssddanbrown added this to the BookStack Beta v0.18 milestone Jun 17, 2017

@ssddanbrown

This comment has been minimized.

Show comment
Hide comment
@ssddanbrown

ssddanbrown Jun 17, 2017

Member

Hi @Abijeet, Thanks again for the above work, For an initial quick look over the code it looks great. Unfortunately I've had limited time recently, Due to moving home and some health issues, so I haven't had a deeper dive into this yet.

The next BookStack release, V0.17, is going to be a cleanup of code display/inputs, which is what I've been slowly working on recently but I've marked this commenting system as being the main feature of the release after that (v0.18). Once v0.17 is out I'll put all my focus on reviewing and merging your work here.

Member

ssddanbrown commented Jun 17, 2017

Hi @Abijeet, Thanks again for the above work, For an initial quick look over the code it looks great. Unfortunately I've had limited time recently, Due to moving home and some health issues, so I haven't had a deeper dive into this yet.

The next BookStack release, V0.17, is going to be a cleanup of code display/inputs, which is what I've been slowly working on recently but I've marked this commenting system as being the main feature of the release after that (v0.18). Once v0.17 is out I'll put all my focus on reviewing and merging your work here.

@Abijeet

This comment has been minimized.

Show comment
Hide comment
@Abijeet

Abijeet Jun 17, 2017

Member

@ssddanbrown - Thanks, the plan sounds good. Take care of your health.

Member

Abijeet commented Jun 17, 2017

@ssddanbrown - Thanks, the plan sounds good. Take care of your health.

@ssddanbrown

This comment has been minimized.

Show comment
Hide comment
@ssddanbrown

ssddanbrown Aug 1, 2017

Member

Hi @Abijeet,
BookStack v0.18 is now the development focus so I've just pulled down your code and played around with the comments. It all works great! Thanks so much for your work here.

I'll merge this into master now. Before merging I'm going to combine the two required database migrations into one and rename it to be the latest to ensure a smooth upgrade for all users. This may break any instances you've developed this comments system on.

I've only lightly skimmed the rest of the code but I'll go through it in detail soon. Before v0.18 is released I'll probably rewrite any angular components here in Vue as I plan to migrate everything else to Vue soon. I also plan on doing some design changes across BookStack for v0.18 so some styling of this system may change.

In regards to setting up the demo data for demo.bookstackapp.com, That's simply done by me manually before taking a database dump so don't worry about having to code that in at all.

Once again, Thank you for your work here.

Member

ssddanbrown commented Aug 1, 2017

Hi @Abijeet,
BookStack v0.18 is now the development focus so I've just pulled down your code and played around with the comments. It all works great! Thanks so much for your work here.

I'll merge this into master now. Before merging I'm going to combine the two required database migrations into one and rename it to be the latest to ensure a smooth upgrade for all users. This may break any instances you've developed this comments system on.

I've only lightly skimmed the rest of the code but I'll go through it in detail soon. Before v0.18 is released I'll probably rewrite any angular components here in Vue as I plan to migrate everything else to Vue soon. I also plan on doing some design changes across BookStack for v0.18 so some styling of this system may change.

In regards to setting up the demo data for demo.bookstackapp.com, That's simply done by me manually before taking a database dump so don't worry about having to code that in at all.

Once again, Thank you for your work here.

@ssddanbrown ssddanbrown merged commit 574ee82 into BookStackApp:master Aug 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alagu

This comment has been minimized.

Show comment
Hide comment
@alagu

alagu Aug 2, 2017

Thanks a ton for this @Abijeet and @ssddanbrown!

alagu commented Aug 2, 2017

Thanks a ton for this @Abijeet and @ssddanbrown!

@alagu

This comment has been minimized.

Show comment
Hide comment
@alagu

alagu Aug 2, 2017

@ssddanbrown Will this be pushed into release sometime soon?

alagu commented Aug 2, 2017

@ssddanbrown Will this be pushed into release sometime soon?

@ssddanbrown

This comment has been minimized.

Show comment
Hide comment
@ssddanbrown

ssddanbrown Aug 2, 2017

Member

@alagu It's marked for the next release so yeah, Milestone here.

As above there's some changes I want to make (In general in BookStack) as part of the next release. Will likely be released later this month.

Member

ssddanbrown commented Aug 2, 2017

@alagu It's marked for the next release so yeah, Milestone here.

As above there's some changes I want to make (In general in BookStack) as part of the next release. Will likely be released later this month.

@Abijeet

This comment has been minimized.

Show comment
Hide comment
@Abijeet

Abijeet Aug 7, 2017

Member

@ssddanbrown - There is a lot of AngularJS code here that'll have to be migrated to VueJS. I'll see if I can help you with that. I've wanted to work with VueJS for some time now, this will give me the opportunity to do so.

Other than the code there are a couple of things I think we should do, please feel free to add your thoughts,

  1. Regarding the demo data, I want to have a page with comments and sub comments displayed on it.
  2. It maybe worth mentioning comments on the website here - https://www.bookstackapp.com/, maybe under powerful features?
Member

Abijeet commented Aug 7, 2017

@ssddanbrown - There is a lot of AngularJS code here that'll have to be migrated to VueJS. I'll see if I can help you with that. I've wanted to work with VueJS for some time now, this will give me the opportunity to do so.

Other than the code there are a couple of things I think we should do, please feel free to add your thoughts,

  1. Regarding the demo data, I want to have a page with comments and sub comments displayed on it.
  2. It maybe worth mentioning comments on the website here - https://www.bookstackapp.com/, maybe under powerful features?
@Abijeet

This comment has been minimized.

Show comment
Hide comment
@Abijeet

Abijeet Aug 7, 2017

Member

@ssddanbrown - For Vue, are you modelling the code in a certain manner? Is there some sample code that I can view and build off of?

Member

Abijeet commented Aug 7, 2017

@ssddanbrown - For Vue, are you modelling the code in a certain manner? Is there some sample code that I can view and build off of?

@ssddanbrown

This comment has been minimized.

Show comment
Hide comment
@ssddanbrown

ssddanbrown Aug 8, 2017

Member

Regarding the demo data, I want to have a page with comments and sub comments displayed on it.

Yeah, That's fine, Once this is in release I'll go through a few of the pages and add comments before snapshotting demo data.

It maybe worth mentioning comments on the website here - https://www.bookstackapp.com/, maybe under powerful features?

Yeah, I agree. In general the features info needs an update as I have not touched that in over a year.

In regards to vue, All vue-based js can be found in the vue folder. Vue logic is put in it's own file then registered in the vues.js file. This system looks for elements of ID's based on that registration then, If a matching element is found, a new Vue instance is created.

The largest example of this currently in place is the search system, With all of the JS vue logic here and the matching HTML found here.

Member

ssddanbrown commented Aug 8, 2017

Regarding the demo data, I want to have a page with comments and sub comments displayed on it.

Yeah, That's fine, Once this is in release I'll go through a few of the pages and add comments before snapshotting demo data.

It maybe worth mentioning comments on the website here - https://www.bookstackapp.com/, maybe under powerful features?

Yeah, I agree. In general the features info needs an update as I have not touched that in over a year.

In regards to vue, All vue-based js can be found in the vue folder. Vue logic is put in it's own file then registered in the vues.js file. This system looks for elements of ID's based on that registration then, If a matching element is found, a new Vue instance is created.

The largest example of this currently in place is the search system, With all of the JS vue logic here and the matching HTML found here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment