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

feat(markdown-docx): add inline-code transformer - #397 #426

Merged
merged 4 commits into from Jul 8, 2021

Conversation

K-Kumar-01
Copy link
Collaborator

Signed-off-by: K-Kumar-01 kushalkumargupta4@gmail.com

Add the roundtrip transformation i.e. CiceroMark<->OOXML transformation for inline-code.

Changes

  • Inline code transformation: CiceroMark<->OOXML
  • Test for the above

Screenshots or Video

c6dd0dc3-7828-4611-be63-73936def02f3

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

Inline code transformation: CiceroMark<->OOXML
Test for the above

Signed-off-by: K-Kumar-01 <kushalkumargupta4@gmail.com>
@K-Kumar-01 K-Kumar-01 changed the title feat(markdown-docx): add inline-code transformer feat(markdown-docx): add inline-code transformer - #397 Jul 7, 2021
Copy link
Member

@algomaster99 algomaster99 left a comment

Choose a reason for hiding this comment

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

Very clean PR. Great work! I have dropped some minor changes.

packages/markdown-docx/src/ToCiceroMarkVisitor.js Outdated Show resolved Hide resolved
Comment on lines 208 to 212
if (runTimeProperties.attributes['w:val'] === 'C45911') {
colorCodePresent = true;
}
} else if (runTimeProperties.name === 'w:highlight') {
if (runTimeProperties.attributes['w:val'] === 'lightGray') {
Copy link
Member

Choose a reason for hiding this comment

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

TemplateMark Dingus uses #C7254E and #F9F2F4 for color and highlight respectively. I think it would make sense if we are consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@algomaster99 We can change the color but for highlighting I am afraid as word supports limited highlighting colors.
Image when color is changed to C7254E.
Screenshot (79)

Should I change the color with respect to above image?

Copy link
Member

Choose a reason for hiding this comment

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

Just making sure here. Highlight colour is the background colour while the simply "colour" is the font colour, right?

Copy link
Member

Choose a reason for hiding this comment

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

MS Word itself doesn't have limited colours for font-colour so it's quite tough to understand changing color to #C7254E is not possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just making sure here. Highlight colour is the background colour while the simply "colour" is the font colour, right?

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MS Word itself doesn't have limited colours for font-colour so it's quite tough to understand changing color to #C7254E is not possible.

I am not sure as when I changed the color in OOXML to #C7254E it gave no errors. So I assumed that it might pe possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@algomaster99
So should we keep the colors as it is? I mean no changes in text or highlight color.

Copy link
Member

Choose a reason for hiding this comment

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

I tried to tweak colours myself and it turns out that you can change the font color to any RGB colour however, highlight colour only takes specific values which are listed in the link I have linked below.

<w:r>
  <w:rPr>
    <w:color w:val="C7254E" />
    <w:shd w:fill="F9F2F4"/>
  </w:rPr>
   <w:t>Inline code</w:t>
</w:r>

You can try to generate an OOXML like above to achieve the desired effect.

Reference: http://officeopenxml.com/WPtextShading.php.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@algomaster99 will update the PR ASAP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Signed-off-by: K-Kumar-01 <kushalkumargupta4@gmail.com>
if (runTimeProperties.attributes['w:val'] === 'C7254E') {
colorCodePresent = true;
}
} else if (runTimeProperties.name === 'w:shd') {
Copy link
Member

Choose a reason for hiding this comment

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

Just comment here why you are using shd instead of highlight.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you give insights here?
According to me

Limited highlighting in word so use shade for highlting.

Copy link
Member

Choose a reason for hiding this comment

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

w:shd tag is used to detect the highlight colour of the text. w:highlight is not used for this purpose because of its ability to render fixed colours. Reference: http://officeopenxml.com/WPtextShading.php.

Signed-off-by: K-Kumar-01 <kushalkumargupta4@gmail.com>
Signed-off-by: Aman Sharma <mannu.poski10@gmail.com>
@algomaster99 algomaster99 merged commit db4176f into algoo-ooxml Jul 8, 2021
@algomaster99 algomaster99 deleted the k-kumar-01/i397/inline-code-transformer branch July 8, 2021 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants