meteorlxy / vssue

:mailbox: A Vue-powered Issue-based Comment Plugin
https://vssue.js.org
MIT License
773 stars 106 forks source link

The comment deleted wouldn't be removed from the page when there is only one comment #4

Closed William17 closed 5 years ago

William17 commented 5 years ago

https://github.com/meteorlxy/vssue/blob/3999dd49e6d0330ed064bba184cd95e68ac8981b/packages/vssue/src/components/VssueComment.vue#L306-L308

meteorlxy commented 5 years ago

@William17 Thanks!

meteorlxy commented 5 years ago

Will, it's not truely a bug. Vssue will always reload comments after deleting a comment:

https://github.com/meteorlxy/vssue/blob/3999dd49e6d0330ed064bba184cd95e68ac8981b/packages/vssue/src/components/VssueComment.vue#L314

Currently, if there are more than one comment on this page, we remove the deleted comment immediately, and then reload comments. If there is only one comment on this page, we don't remove it immediately, but wait for realoding comments.

William17 commented 5 years ago

You are right. Below is the bug https://github.com/meteorlxy/vssue/blob/3999dd49e6d0330ed064bba184cd95e68ac8981b/packages/vssue/src/components/VssueComment.vue#L295-L315

// line 295
this.vssue.status.isLoadingComments = true
// ...
// line 314
await this.vssue.getComments() 

And then in getComments method, since this.vssue.status.isLoadingComments = true, it just returns https://github.com/meteorlxy/vssue/blob/3999dd49e6d0330ed064bba184cd95e68ac8981b/packages/vssue/src/VssueStore.ts#L222-L225

meteorlxy commented 5 years ago

Oh, that's it.

The flag here is to prevent reloading comments when deleting a comment. If user changed page / perPage before the deletion finished, there might have some errors:

https://github.com/meteorlxy/vssue/blob/3999dd49e6d0330ed064bba184cd95e68ac8981b/packages/vssue/src/components/VssueComment.vue#L311-L312

But yes it won't reload comments as expected because of the flag. Do you have any advice?

William17 commented 5 years ago

I think we should prevent the user from changing the page/perPage when deleting a comment. In other words, put the flag in the UI layer rather than the API layer.

meteorlxy commented 5 years ago

The isLoadingComments flag will disable pagination, but will disbale reloading as well...

Do you mean to set anthor flag to disable pagination in UI layer?


I have some solutions, but cannot determine which one is more reasonable:

  1. Unset the isLoadingComments after deletion finished:
      this.vssue.status.isLoadingComments = true

      const success = await this.vssue.deleteComment({
        commentId: this.comment.id,
      })

      this.vssue.status.isLoadingComments = false

      if (success) {
        // ...
  1. Use nextTick to reload comments:
    if (success) {
      this.$nextTick(async () => {
        // decrease count immediately
        this.vssue.comments!.count -= 1

        // if there are more than one comment on this page, remove the deleted comment immediately
        if (this.vssue.comments!.data.length > 1) {
          this.vssue.comments!.data.splice(this.vssue.comments!.data.findIndex(item => item.id === this.comment.id), 1)
        }

        // if the page count should be decreased, change the query param to trigger comments reload
        if (this.vssue.query.page > 1 && this.vssue.query.page > Math.ceil(this.vssue.comments!.count / this.vssue.query.perPage)) {
          this.vssue.query.page -= 1
        } else {
          await this.vssue.getComments()
        }
      })
    }
  1. Set a force flag to reload comments:
// VssueStore.ts
 async getComments (force?: boolean = false): Promise<VssueAPI.Comments | void> { 
   try { 
     if (!this.API || !this.issue || (this.status.isLoadingComments && !force)) return 
// VssueComment.vue
await this.vssue.getComments(true)
meteorlxy commented 5 years ago

I temporarily use force flag to fix this. Feel free to comment if you have further advice.

William17 commented 5 years ago

In my opinion, the most elegant way is using a flag like isUpdatingComments. When it's true, all actions are disabled. We can only set the flag in components. The methods in VsseStore should not know the existance of the flag.

meteorlxy commented 5 years ago

@William17 Let's move to #5 for discussion

meteorlxy commented 5 years ago

fixed in v0.5.2