DEV-24325 Add support for comment replies #21
Conversation
src/components/Paragraph.ts
Outdated
| // For regular paragraphs this identifier is not required. | ||
| // It is when comments have replies. These "links" (X comment is a reply of Y comment) | ||
| // are handled via this identifier. | ||
| #id: string | null = null; |
There was a problem hiding this comment.
If this 'id' is specific to that case and, consequently, only to the 'paraId' attribute, shouldn't it be called 'paraId' directly?
There was a problem hiding this comment.
The paraId is the identifier of the paragraph. I used id to remove the redundant para part.
src/components/Paragraph.ts
Outdated
| * This identifier is used by comment replies. | ||
| */ | ||
| public set id(id: number | string) { | ||
| this.#id = typeof id === 'string' ? id : toHex(id); |
There was a problem hiding this comment.
This is confusing for me. Word provides (or requires) the value of paraId as a string and in hexadecimal, right? If so, this value should always be a string. It is FO who has to provide the value accordingly. Maybe I'm wrong, but docxml doesn't usually do type conversions and such (apart from dimensions).
There was a problem hiding this comment.
I store a hexadecimal value (instead of a number) just to simplify a bit everything. This setter allows a number or an string just to simplify setting a value. When importing a paragraph (fromNode) the value will be already in hexadecimal, so the string is used in that case. When creating a reply it may be needed to set an identifier, in that case it is easier to give this function a number.
In all cases the hexadecimal value is always handled in this class, so no external classes need to handle that.
Maybe it is best to create a new Id type, similar to Length, so we can handle multiple formats at once.
There was a problem hiding this comment.
I've implemented a new Id type.
| /** | ||
| * Creates an XML DOM node for this component instance. | ||
| */ | ||
| public override async toNode(ancestry: ComponentAncestor[]): Promise<Node> { | ||
| /** | ||
| * For some reason, MSWord requires the paraId attribute to have the w14 namespace, and at the |
There was a problem hiding this comment.
This scares me a bit... Could we find out something more in depth? It's the first thing I see in the project where ‘"w14" or ".w15" is used... wouldn't "w" be enough?
There was a problem hiding this comment.
Clarification: It scares me because I have the feeling that it could lead to compatibility problems, but I can't prove my thoughts right now either.
There was a problem hiding this comment.
Doing a quick test my Word uses those namespaces, I guess they are correct. Sorry for my monologue hahaha
There was a problem hiding this comment.
I couldn't find a reason why this happens. MSWord things ¯_(ツ)_/¯.
|
👀 This is awesome guys! |
Comments (and ranges) now have a new
parentIdproperty so they're internally linked together. The way replies work in OOXML is wild:CommentsExtended.xml.a. This one is pretty simple actually, contains
w15:commentExnodes with some attributes.b.
w15:paraId. This one is the identifier of the paragraph inside the Comment (remember that aw:commenthas aw:pchild node).c.
w15:paraIdParent. This one is optional, is the identifier of the parent paragraph.w:commentnode, not by thew:commentthemselves 🤡.A few changes:
idproperty. This one translates tow14:paraIdwhen converted to a node.w15:paraIdattribute I'm using the Comment identifier, but converted to a 4 bytes hexadecimal value (again, 🤡).