Remove remaining occurrences of "@xxx" from non-pdf versions #595
Conversation
| Thank you @samuelfangjw! However, in hindsight -- would it not be better to bypass the transformation from XML tags to @xxx, etc.? initially, I used a simpler marker directly in the code in the snippets, but then we moved to a proper XML tag. |
Agreed! It would be much neater that way. I originally didn't want to make any unnecessary changes that might affect the pdf version (while we are trying to get the book published), hence this ugly stopgap solution. I took a quick look, I believe bypassing the transformation may not be as simple as it seems, since in the pdf version "@yyy" is transformed into either However, seems like it would be pretty simple to limit the transformation of XML tags to just the pdf version for now. Would this be better? I'll take a closer look at this and make any necessary changes tomorrow! |
| jsRunSnippet = jsSnippet; | ||
| } | ||
| } | ||
| const codeArr = []; |
There was a problem hiding this comment.
Dead code, variable not used
| SHORT_SPACE_AND_ALLOW_BREAK: "@yyy" // will be replaced in processSnippet | ||
| }; | ||
| | ||
| export const replaceTagWithSymbol = (node, writeTo, type = "default") => { |
There was a problem hiding this comment.
Add parameter type. SHORT_SPACE and SHORT_SPACE_AND_ALLOW_BREAK are replaced if type === "pdf"
| Hi @TobiasWrigstad, I've made some changes:
I think it would be great if we could get rid of the use of the placeholder values "@xxx" and "@yyy" entirely, but I feel it might be more trouble than it's worth at this point (Not a straightforward swap since "@yyy" is replaced either by Could you kindly help to review the PR, thanks in advance! I've also attached the pdf version created by |
| Great @samuelfangjw! I will be able to look at this later today. |
| SHORT_SPACE: "@xxx", // will be replaced in processSnippet depending on rendering target (PDF, HTML, etc.) | ||
| SHORT_SPACE_AND_ALLOW_BREAK: "@yyy", // will be replaced in processSnippet depending on rendering target (PDF, HTML, etc.) | ||
| SHORT_SPACE: "", | ||
| SHORT_SPACE_AND_ALLOW_BREAK: "", |
There was a problem hiding this comment.
Would it make sense to replace these with "\n" right here?
There was a problem hiding this comment.
Ah, wait. That's the effect of replacing the tag by "", presumably.
There was a problem hiding this comment.
Thanks @TobiasWrigstad for the review! Yes, I would have thought "\n" would have been more appropriate here, but it seems to add an extra line break (replaced by two blank lines instead of one).
Original: replace by "@xxx":
Replace with "\n":
Replace with "":
| I made some comments above and then deleted them. I made a very stupid mistake and was looking at another file than yours dues to browser renaming it on download... |
2 participants
Closes #591
Accounted for possibility that "@xxx" is not followed by newline character.ReplacedSHORT_SPACEandSHORT_SPACE_ALLOW_BREAKwith newline character in non-pdf versions.processSnippetJsEDIT: