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

Use <br> to separate lines of code block #1176

Merged
merged 3 commits into from
Apr 7, 2020

Conversation

openorclose
Copy link
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Bug fix

What is the rationale for this request?
Fixes #1173
Fixes #1125

What changes did you make? (Give an overview)

In #991 , to handle empty lines, an unprintable character was added to make the span take up space. This causes problems for code copying.

Instead of that, let's make the span be inline-block and use <br> to put them on new lines.

Provide some example code that this change will affect:

```
some code





new lines above
```

This should now display correctly, and not have any new line characters inside.

Is there anything you'd like reviewers to focus on?
Testing instructions:

Check that multiple lines work, and that copying them also works

Proposed commit message: (wrap lines at 72 characters)

Use <br> and inline-block to separate spans.

Adding extra unprintable characters cause code copying to fail.

Let's not use them, and manually separate
them by using <br> instead.

@openorclose openorclose added the pr.BugFix 🐛 Fixes correct a programming error/assumption label Apr 2, 2020
@ang-zeyu
Copy link
Contributor

ang-zeyu commented Apr 2, 2020

@openorclose
Copy link
Contributor Author

Different fix may be needed:

code fence

https://deploy-preview-1176--markbind-master.netlify.com/userguide/formattingcontents#line-highlighting

Oh what browser is that?

I'm using firefox 73 and it looks ok:

image

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Apr 2, 2020

Oh what browser is that?

chrome v80, seems to be broken on edge ( I'm on a pretty old version though ) as well

Looks fine on firefox for me as well 👍

@openorclose
Copy link
Contributor Author

Oh what browser is that?

chrome v80, seems to be broken on edge ( I'm on a pretty old version though ) as well

Looks fine on firefox for me as well

Yeah, have to think of a different fix...

Changing empty lines to a single space probably wouldn't work also, since these extra spaces might affect how the program runs.

@alyip98
Copy link
Contributor

alyip98 commented Apr 4, 2020

@openorclose does this fix work?

I moved the line breaks inside the spans and made the <span> display:block

@openorclose
Copy link
Contributor Author

Yeah seems to work. Thanks!

# Conflicts:
#	test/functional/test_site/expected/requirements/UserStories._include_.html
Copy link
Contributor

@alyip98 alyip98 left a comment

Choose a reason for hiding this comment

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

Looks like LGTM to me on all 3 major browsers!

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

OK. Just a non-blocking question, can't remember whether this was asked: why did we use <span> instead of <div>?

@yamgent yamgent added this to the v2.13.2 milestone Apr 5, 2020
@openorclose
Copy link
Contributor Author

OK. Just a non-blocking question, can't remember whether this was asked: why did we use <span> instead of <div>?

I chose it rather arbitrarily, but if i were to give a reason it would be that <span> can only contain inline elements which seems more appropriate for showing a line of code.

@yamgent yamgent merged commit e61cbc3 into MarkBind:master Apr 7, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Apr 9, 2020
* 'master' of https://github.com/MarkBind/markbind: (41 commits)
  Document adding new site content in DG (MarkBind#1153)
  Add relative date feature (MarkBind#908)
  Use <br> to separate lines of code block (MarkBind#1176)
  Parse popovers for footnotes (MarkBind#1155)
  Resolve comments
  Expand test extensions and fix whitespace checks (MarkBind#1156)
  Address comments
  Upgrade js-beautify and provide option to turn it off (MarkBind#1116)
  Normalize inline puml line ending before hashing (MarkBind#1174)
  Update tests (MarkBind#1178)
  Remove fixed bugs from test\functional\test_site\bugs\index.md` (MarkBind#1148)
  Fix bug in Search for UG and DG (MarkBind#1147)
  Add inline puml support (MarkBind#1100)
  Fix hrefs for headings with angular brackets (MarkBind#1089)
  Update tests for 2.13.1 (MarkBind#1169)
  2.13.1
  Update vue-strap version to v2.0.1-markbind.39
  Fix fontawsome icons don't show underlines to indicate modal/tooltip (MarkBind#1133)
  2.13.0
  Update test files
  ...
@jamessspanggg
Copy link

@openorclose code copying for safari:

Kapture 2020-04-10 at 23 33 06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr.BugFix 🐛 Fixes correct a programming error/assumption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra unprintable chars in code blocks Copying code in code blocks not working properly
5 participants