-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix extra empty line issue on tutorial #663
Conversation
@swift-ci please test |
@@ -192,7 +192,7 @@ function duplicateMultilineNode(element) { | |||
|
|||
// wrap each new line with the current element's class | |||
const result = getLines(element.innerHTML) | |||
.reduce((all, lineText) => `${all}<span class="${className}">${lineText || '\n\n'}</span>\n`, ''); | |||
.reduce((all, lineText) => `${all}<span class="${className}">${lineText || ''}</span>\n`, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. This change likely fixes one bug but it unfortunately re-introduces another that we previously had. These newlines were added to fix a bug where empty lines in a multiline Swift string were accidentally getting stripped.
With this code listing as an example:
let multiline = """
Needs
Spaces
Between
Lines
"""
On main, this gets rendered how it looks there.
On this branch, this gets rendered as follows:
let multiline = """
Needs
Spaces
Between
Lines
"""
Ideally we could find a solution that fixes #662 without re-introducing this other bug, although I know it's tricky because of the complex logic we have to handle line numbers and syntax highlighting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #502 was the PR for the bug fix that this change might accidentally re-introduce for context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. This PR does not re-introduces the bug you mention. It passed the existing test and I tested this form and confirm it locally.
Because the original use nothing. #502 add
|| "\n\n"
. But actually add|| ""
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably re-visit the way we render line numbers and the css styling around that.
We have a display:flex with a column wrap on the <code>
element, which is probably not a good appraoch, since we are messing up the white space handling entirely, hence why I had to add that double /n/n
while looping over the multi-lines.
There are two good ways to do this from my research:
- The traditional table rendering each line approach
- Move line numbers to a list after the code and leave some space for them.
Each has it's PROs and CONs.
Close. Prefer to use #667 |
Bug/issue #, if applicable:
Close #662
Summary
See the full context on #622
Another way to fix the problem is changing the logic here
src/components/ContentNode/CodeListing.vue
A later fix to implement a clipboard button may help lift the issue.
#449
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test
, and it succeeded