Comment section: Use int for part hash. #10965
Conversation
|
When I click somewhere in empty area and then try to insert comment with keyboard shortcut I am getting |
Check container existence before setting edit mode. Signed-off-by: Gökay Şatır <[email protected]> Change-Id: I4c3a4a27c515f1ee3c44c501b5a64550128c50bd
Fetching the comments can be initiated also by us. Signed-off-by: Gökay Şatır <[email protected]> Change-Id: I006f0f8bd264f72356f13e8c77269279d7a5001d
gokaysatir
force-pushed
the
private/gokay/latlng-6
branch
from
6da2971 to
241edd6
Compare
| @@ -120,7 +120,7 @@ export class Comment extends CanvasSectionObject { | |||
| this.sectionProperties.showSelectedCoordinate = true; // Writer. | |||
|
|
|||
| if (app.map._docLayer._docType === 'presentation' || app.map._docLayer._docType === 'drawing') { | |||
| this.sectionProperties.parthash = this.sectionProperties.data.parthash; | |||
| this.sectionProperties.parthash = parseInt(this.sectionProperties.data.parthash); | |||
There was a problem hiding this comment.
if parthash inside data should be always a number, shouldn't we do it once when we receive that data and assign to sectionProperties? so we don't have to convert again.
Can we have number type assigned to it so TS compiler will warn about incorrect usage?
There was a problem hiding this comment.
I think this is the right place. This is constructor and sectionProperties.data is an input parameter.
|
Can we have some cypress test here too? |
| @@ -2366,17 +2366,6 @@ export class CommentSection extends app.definitions.canvasSectionObject { | |||
| this.disableLayoutAnimation = true; | |||
| var comment; | |||
| if (Comment.isAnyEdit()) { | |||
| this.map.uiManager.showConfirmModal( | |||
There was a problem hiding this comment.
So probably we should detect when we initiate import or not (as optional param?)
Then we can show the dialog only when needed
There was a problem hiding this comment.
We have below lines:
if (refreshAnnotation)
app.socket.sendMessage('commandvalues command=.uno:ViewAnnotations');
I guess that re-import breaks things. My take is to keep this modal out for now. We have onACKComment and it also seems to do the job.
There was a problem hiding this comment.
If it is only for now, then would be good to keep strings so we will not lost translations already done.
Could you move it to separate method (not used) - if that's the case. If not we can delete.
There was a problem hiding this comment.
In fact, i think refreshAnnotation boolean is somehow needed and it re-imports all the comments. In that case, "importComments" is not something that is called at initialization. All the comments are re-imported when that function is called. Including the one we just created and still editing. So i think we can't have that confirmation modal there.
We need to remove it. Then we can have a better look at the function calls and add something else i guess.
|
comments are working as expected now |
I will check and hopefully add a Cypress test soon. |
Check container existence before setting edit mode.
Fixes adding comment to wrong part in Impress.