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

Improved autosaved state for comments #7445

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rparth07
Copy link
Contributor

Change-Id: I9ad6c36a393ef5d895ee463f44569c52f1c8fd61

Checklist

  • Code is properly formatted
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

Copy link
Contributor

@pedropintosilva pedropintosilva left a comment

Choose a reason for hiding this comment

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

Thanks @rparth07

it seems that now the comment (edit mode) is visible for other users

and I also get when opening the same document and trying to comment in two different browsers

17:41:10.008 Uncaught TypeError: this._viewInfo[viewid] is undefined
    getViewName Map.js:872
    insertComment CommentListSection.ts:8
    _insertAnnotationControl Control.NotebookbarBuilder.js:936
    jQuery 2
6 Map.js:872:2

Also it the user comes back and doesn't press any button and clicks in another part of the document -> it's then impossible to edit the comment

@rparth07
Copy link
Contributor Author

rparth07 commented Nov 13, 2023

it seems that now the comment (edit mode) is visible for other users

Sure, let's clarify the intended behavior. If I understand correctly, you're suggesting that the comment in edit mode should not be visible to other users once the focus is lost. Is that the expected behavior we aim for?

and I also get when opening the same document and trying to comment in two different browsers

17:41:10.008 Uncaught TypeError: this._viewInfo[viewid] is undefined
    getViewName Map.js:872
    insertComment CommentListSection.ts:8
    _insertAnnotationControl Control.NotebookbarBuilder.js:936
    jQuery 2
6 Map.js:872:2

I couldn't replicate the error on my end. Could you please provide detailed steps to reproduce the issue? Additionally, are there specific browsers where this problem occurs?

Also it the user comes back and doesn't press any button and clicks in another part of the document -> it's then impossible to edit the comment

The described behavior does sound unusual. Could you please provide more details on this issue? Also, do you think this behavior is related to the error mentioned earlier?

Any insights you can share will help me in thoroughly understanding and resolving this. Thank you for your assistance @pedropintosilva .

@rparth07 rparth07 force-pushed the comment-autosave-state-improvement branch from 385b4d3 to 4345b46 Compare March 14, 2024 03:59
@eszkadev
Copy link
Contributor

@rparth07 hi, are you still working on this PR? :) It requires rebase as we have conflicts with newer commits.

Would be good to put few sentences into commit message:

  • what issue does it fix?
  • how to test that it is fixed?

@lpranam could help here as he knows best the comments and autosave

@eszkadev eszkadev added the draft label Oct 22, 2024
@Darshan-upadhyay1110
Copy link
Contributor

@rparth07 can you please rebase and solve the conflicts here ?

@rparth07
Copy link
Contributor Author

Thanks @eszkadev @Darshan-upadhyay1110 will do:)

@rparth07 rparth07 force-pushed the comment-autosave-state-improvement branch 2 times, most recently from 84f1a41 to 5542fa1 Compare November 2, 2024 10:38
@lpranam
Copy link
Member

lpranam commented Nov 6, 2024

Merge commits are not allowed...

@rparth07 rparth07 force-pushed the comment-autosave-state-improvement branch from eb95f46 to feb0023 Compare November 6, 2024 15:50
@rparth07
Copy link
Contributor Author

rparth07 commented Nov 6, 2024

Merge commits are not allowed...

sorry, It was by mistake!

@rparth07
Copy link
Contributor Author

rparth07 commented Nov 6, 2024

I resolved the merge conflicts and updated the relevant test cases for autosaved comments. However, in the multi-user Cypress tests, three test cases related to reply autosave are currently failing.

Upon further investigation, I found a potential issue (#10429) that may be causing these test failures. I believe that addressing this issue will likely resolve the failing test cases.

@eszkadev
Copy link
Contributor

Tests are now passing. @rparth07 will you have some time to update this PR and rebase on top of master?

@rparth07
Copy link
Contributor Author

Tests are now passing. @rparth07 will you have some time to update this PR and rebase on top of master?

Sure, will do it by today.

@rparth07 rparth07 force-pushed the comment-autosave-state-improvement branch from feb0023 to 9e5b8d0 Compare December 12, 2024 17:05
@eszkadev
Copy link
Contributor

please rebase, we have API change again...

This commit addresses issue CollaboraOnline#7211 by enhancing the user experience for the autosaved comments feature. The changes ensure users can easily recognize autosave status without increasing widget height.

To test the fix, manually verify the autosave behavior by changing focus to another browser tab and checking that the comment remains editable with clear autosaved feedback.

Signed-off-by: Parth Raiyani <[email protected]>
Change-Id: I9ad6c36a393ef5d895ee463f44569c52f1c8fd61
Signed-off-by: Parth Raiyani <[email protected]>
Change-Id: I0ebb1c8805d04e687e16529ba39742f00577dd0c
@rparth07 rparth07 force-pushed the comment-autosave-state-improvement branch from 9e5b8d0 to e5ff930 Compare December 19, 2024 18:27
@eszkadev
Copy link
Contributor

this can have conflicts with #10772
let's wait for the other to be merged first as tests here are not passing it seems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Improve autosaved state for comments
5 participants