feat: add text and emphasis transformers - #397 #401

K-Kumar-01 · 2021-06-10T15:40:58Z

Signed-off-by: k-kumar-01 [email protected]

Reference #397

Changes

Rules and helpers for the conversion
Complete roundtripping for text and emphasis(CiceroMark->OOXML->CiceroMark)
Test for the above

Author Checklist

Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
Vital features and changes captured in unit and/or integration tests
Commits messages follow AP format
Extend the documentation, if necessary
Merging to master from fork:branchname

Rules and helpers for the conversion Complete roundtripping for text and emphasis(CiceroMark->OOXML->CiceroMark) Test for the above Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 · 2021-06-10T15:41:10Z

@algomaster99 Requesting your review.

algomaster99

Once you create the .cta archive, I can give more reviews on the logic for emphasis visitor. For now, see comments.

Download VSCode extension for JS formatting so that your white spaces are consistent. Make sure you set the rules which are consistent with the codebase.
Push your branch to this repository itself (not your fork). Makes it easier for me to fetch and test.

EDIT:
Try creating the .cta archive. Otherwise, I can test without it too.

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.js

packages/markdown-docx/test/data/ciceroMark/ciceroMark.js

packages/markdown-docx/src/rules.js

packages/markdown-docx/test/data/ciceroMark/ciceroMark.js

K-Kumar-01 · 2021-06-10T19:32:28Z

Push your branch to this repository itself (not your fork). Makes it easier for me to fetch and test.

Can I do this from next PR?

algomaster99 · 2021-06-10T19:37:19Z

@K-Kumar-01 yes. I forgot to add "do this from the next PR".

Naming changed to improve readability and understandability Files renamed for better understanding Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 · 2021-06-10T20:06:36Z

@algomaster99
I have updated the code.
Also here is the cta.
test-and-emphasis.zip
Let me know if there are any more changes required.

algomaster99

Dropped a few comments.

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.js

packages/markdown-docx/src/CiceroMarkToOOXMLRules.js

algomaster99 · 2021-06-10T21:08:36Z

packages/markdown-docx/src/OoxmlTransformer.js

+            if(element.elements[0].name==='w:rPr'){
+                let emphFound = element.elements[0].elements.some(subElement=>{
+                    return subElement.name==='w:i' && subElement.attributes['w:val'] === 'true';
+                });
+                if(emphFound){
+                    return {
+                        $class: `${NS_PREFIX_CommonMarkModel}Emph`,
+                        nodes:[...this.deserializeElements(element.elements)]
+                    };
+                }
+            }


I will see to this later in the morning.

Improve the spacing by using a good formatter such as prettier. You can integrate its extension in VSCode. Make sure the rules your configure are almost consistent with the entire codebase.

Documentation update Signed-off-by: k-kumar-01 <[email protected]>

Declares OOXML as instance variable Signed-off-by: k-kumar-01 <[email protected]>

algomaster99

Final reviews then we are good to merge.

packages/markdown-docx/src/OoxmlTransformer.js

algomaster99 · 2021-06-11T07:07:56Z

packages/markdown-docx/src/OoxmlTransformer.js

+            if(element.elements[0].name==='w:rPr'){
+                let emphFound = element.elements[0].elements.some(subElement=>{
+                    return subElement.name==='w:i' && subElement.attributes['w:val'] === 'true';
+                });
+                if(emphFound){
+                    return {
+                        $class: `${NS_PREFIX_CommonMarkModel}Emph`,
+                        nodes:[...this.deserializeElements(element.elements)]
+                    };
+                }
+            }


Improve the spacing by using a good formatter such as prettier. You can integrate its extension in VSCode. Make sure the rules your configure are almost consistent with the entire codebase.

algomaster99 · 2021-06-11T07:08:18Z

packages/markdown-docx/src/OoxmlTransformer.js

+                let emphFound = element.elements[0].elements.some(subElement=>{
+                    return subElement.name==='w:i' && subElement.attributes['w:val'] === 'true';
+                });
+                if(emphFound){


emphasisedTextFound

@algomaster99
I used the default JS and Ts formatters above.
Also, for prettier, there was no config file I didn't use it. I will use the default config for prettier that match the codebase.

Yes, use prettier. It should take care of everything. Just make sure it doesn't change very unrelated things in codebase. For example, removes all the ;. This is why I am asking you to write a config file. You can check the docs on how to write one. It's very straightforward.

@algomaster99
I have formatted using prettier. Let me know if there are any more changes required.

Sure. Go ahead. Do that in a separate PR though. And then we can merge the changes with this PR.

@algomaster99 I didn't made any drastic prettier change. Only small area of code needed proper spacing so I don't think it would need a PR.

Variable name changed Spacing improved Signed-off-by: k-kumar-01 <[email protected]>

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.test.js

algomaster99 · 2021-06-11T08:13:27Z

packages/markdown-docx/src/CiceroMarkToOOXMLRules.js

+ * @returns {string} OOXML tag for the text
+ */
+const TEXT_RULE = (value) => {
+    return `<w:r><w:t xml:space="preserve">${sanitizeHtmlChars(value)}</w:t></w:r>`;


Any reason you omitted the spaces in these tags and not the tags in wrapAroundDefaultDocxTags?

@algomaster99 These tags are not used for conversion. So any space between them will act as ' <value>' instead of '<value>'. However, in wrapAroundDefaultDocxTags we do not check for such. So no omission of spaces there.

So any space between them will act as ' ' instead of ''

Any problems with that? This worked fine in the Word add-in. I think the parser we use, xml-js, is also smart enough to ignore such spaces.

@algomaster99
It included the spaces also in values of nodes if it is done similar to add-in. So i dedcided to do it in later way. I also posted pn Slack regarding this. I will also reconfirm it.

@algomaster99

Changed the EMPHASIS_RULE and failed test

By value, do you mean - <w:value /> or <w:t>value</w:t>?

I read your post on slack and I thought it was causing some problems for you. But I don't think there is any issue as I just tried on my local with spaces and breaks.

If you think the size of the string (in bytes) is an issue, you can add a package to uglify an OOXML string after transformation is done.

That is because you literally put a space before and after your value. I am just asking to format the rest of the nodes.

Let's discuss this in the meeting.

By value, do you mean - <w:value /> or <w:t>value</w:t>?

I mean `<w:t>value</w:t>

I read your post on slack and I thought it was causing some problems for you. But I don't think there is any issue as I just tried on my local with spaces and breaks.

Interesting, as it gave me errors (shown above).

If you think the size of the string (in bytes) is an issue, you can add a package to uglify an OOXML string after transformation is done.

We can have a discussion about this in the meeting.

algomaster99 · 2021-06-11T08:13:56Z

packages/markdown-docx/src/CiceroMarkToOOXMLRules.js

+ * @returns {string} OOXML tag for the emphasised text
+ */
+const EMPHASIS_RULE = (value) => {
+    return `<w:r><w:rPr><w:i w:val="true" /></w:rPr><w:t>${sanitizeHtmlChars(value)}</w:t></w:r>`;


w:val="true" is not needed like I reasoned before.

@algomaster99
Done

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.test.js

Remove attribute from <w:i> ooxml tag Signed-off-by: k-kumar-01 <[email protected]>

algomaster99 · 2021-06-11T08:37:51Z

Can you answer this first?

Also, be super clear about what you're testing. In some places, it is text-and-emphasis and in some places, it's paraAndEmphasis. It would cause confusion to someone who is new to this repository.

Yes, they are required. The tests are failing if I do not add JSON.parse.

Okay, right! Better to just parse it once and comment why you needed to parse it.

K-Kumar-01 · 2021-06-11T09:33:39Z

@algomaster99

Okay, right! Better to just parse it once and comment why you needed to parse it.

The comment I made is: converts from string to JSON object.

Should I do a final push? I have updated the names to textAndEmphasis to be more clear.

algomaster99 · 2021-06-11T09:36:16Z

Address this comment and we would be good to go.

K-Kumar-01 · 2021-06-11T09:39:20Z

Address this comment and we would be good to go.

@algomaster99
The changes for this were already made by me in this commit.

Should i push the final changes?

algomaster99 · 2021-06-11T09:42:46Z

Your solution to avoid the test failing because of spaces is at the expense of making the code less readable. I explained a better solution above. Read it if you have time otherwise, we will discuss in the meeting.

Don't push for now.

OOXML spacing changes Signed-off-by: k-kumar-01 <[email protected]>

algomaster99

I did not notice the formatting in the meeting. Just push this and I will merge.

algomaster99 · 2021-06-11T11:31:13Z

packages/markdown-docx/src/CiceroMarkToOOXML/rules.js

+    return `<w:r>
+        <w:rPr>
+            <w:i />
+        </w:rPr>
+        <w:t>${sanitizeHtmlChars(value)}</w:t>
+    </w:r>`;


If you're putting efforts to indent them, do them properly.

return ` <w:r> <w:rPr> <w:i /> </w:rPr> <w:t>${sanitizeHtmlChars(value)}</w:t> </w:r> `;

Same goes for above.

@algomaster99 Done

Signed-off-by: k-kumar-01 <[email protected]>

algomaster99 · 2021-06-11T11:38:15Z

You accidentally pushed the prettier configuration too. I advise you to never use git add ..

K-Kumar-01 · 2021-06-11T11:44:07Z

You accidentally pushed the prettier configuration too. I advise you to never use git add ..

@algomaster99
How can we remove it?

refactor: formattingg rules - accordproject#397 Signed-off-by: k-kumar-01 <[email protected]>

algomaster99

Great work!

feat: add text and emphasis transformers - accordproject#397

9b0decd

Rules and helpers for the conversion Complete roundtripping for text and emphasis(CiceroMark->OOXML->CiceroMark) Test for the above Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 requested a review from algomaster99 June 10, 2021 15:40

algomaster99 suggested changes Jun 10, 2021

View reviewed changes

feat: add text and emphasis transformers - accordproject#397

e43b15a

Naming changed to improve readability and understandability Files renamed for better understanding Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 requested a review from algomaster99 June 10, 2021 20:04

algomaster99 suggested changes Jun 10, 2021

View reviewed changes

K-Kumar-01 added 2 commits June 11, 2021 10:37

feat: add text and emphasis transformers - accordproject#397

f875a96

Documentation update Signed-off-by: k-kumar-01 <[email protected]>

feat: add text and emphasis transformers - accordproject#397

77a0e00

Declares OOXML as instance variable Signed-off-by: k-kumar-01 <[email protected]>

algomaster99 suggested changes Jun 11, 2021

View reviewed changes

feat: add text and emphasis transformers - accordproject#397

d5f5b36

Variable name changed Spacing improved Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 requested a review from algomaster99 June 11, 2021 08:03

algomaster99 suggested changes Jun 11, 2021

View reviewed changes

K-Kumar-01 commented Jun 11, 2021

View reviewed changes

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.test.js Outdated Show resolved Hide resolved

feat: add text and emphasis transformers - accordproject#397

18fb1fd

Remove attribute from <w:i> ooxml tag Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 requested a review from algomaster99 June 11, 2021 08:33

refactor: export transformers from index - accordproject#397

1771d6b

OOXML spacing changes Signed-off-by: k-kumar-01 <[email protected]>

algomaster99 suggested changes Jun 11, 2021

View reviewed changes

refactor: formatting rules - accordproject#397

6c911c2

Signed-off-by: k-kumar-01 <[email protected]>

end-

58482a7

refactor: formattingg rules - accordproject#397 Signed-off-by: k-kumar-01 <[email protected]>

algomaster99 approved these changes Jun 11, 2021

View reviewed changes

algomaster99 merged commit a23129f into accordproject:algoo-ooxml Jun 11, 2021

K-Kumar-01 mentioned this pull request Jun 11, 2021

refactor: export transformers from markdown-docx root #402

Merged

5 tasks

algomaster99 pushed a commit that referenced this pull request Jun 17, 2021

feat: add text and emphasis transformers - #397 (#401)

7651099

algomaster99 pushed a commit that referenced this pull request Jun 17, 2021

feat: add text and emphasis transformers - #397 (#401)

50c8d35

feat: add text and emphasis transformers - #397 #401

feat: add text and emphasis transformers - #397 #401

Conversation

K-Kumar-01 commented Jun 10, 2021 • edited by algomaster99 Loading

Changes

Author Checklist

K-Kumar-01 commented Jun 10, 2021

algomaster99 left a comment • edited Loading

Choose a reason for hiding this comment

K-Kumar-01 commented Jun 10, 2021

algomaster99 commented Jun 10, 2021

K-Kumar-01 commented Jun 10, 2021

algomaster99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K-Kumar-01 Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 commented Jun 11, 2021 • edited Loading

K-Kumar-01 commented Jun 11, 2021

algomaster99 commented Jun 11, 2021

K-Kumar-01 commented Jun 11, 2021 • edited Loading

algomaster99 commented Jun 11, 2021 • edited Loading

algomaster99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 commented Jun 11, 2021

K-Kumar-01 commented Jun 11, 2021

algomaster99 left a comment

Choose a reason for hiding this comment

K-Kumar-01 commented Jun 10, 2021 •

edited by algomaster99

Loading

algomaster99 left a comment •

edited

Loading

K-Kumar-01 Jun 11, 2021 •

edited

Loading

algomaster99 commented Jun 11, 2021 •

edited

Loading

K-Kumar-01 commented Jun 11, 2021 •

edited

Loading

algomaster99 commented Jun 11, 2021 •

edited

Loading