Skip to content

Conversation

@nbriannl
Copy link
Contributor

@nbriannl nbriannl commented Mar 21, 2020

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

• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [X] Enhancement to an existing feature
• [ ] Other, please explain:

Fixes #1119

What is the rationale for this request?

It would be nice to support inline markdown formatting as we support it in other places.

What changes did you make? (Give an overview)

Render heading markdown-it-attr as inline markdown using markdownIt.renderInline(heading)

If heading contains rendered inline markdown, increase line height because default line-height will make the headings crowded. As seen below

with line-height: 1.5
image

with lint-height: 1
image

Added tests as well.

Provide some example code that this change will affect:

```html {heading="### Heading level 3"}
<foo></foo>
```

```html {heading="Line breaks<br>Are possible<br>[And links!](https://markbind.org)"}
<foo></foo>
```

```html {heading="**Bold**, _Italic_, ___Bold and Italic___, ~~Strike through~~, :+1: :exclamation: :x: :construction:"}
<foo></foo>
```

```html {heading="****Super Bold****, ++Underline++, ==Highlight==, %%Dim%%"}
<foo></foo>
```

```html {heading="<blockquote>Using blockquote tag</blockquote>"}
<foo></foo>
```

```html {heading="![](https://markbind.org/images/logo-lightbackground.png)"}
<foo></foo>
```

```html {heading="@[youtube](http://www.youtube.com/watch?v=v40b3ExbM0c)"}
<foo></foo>
```

- Can't support `inline code` text style (because it would break markdown-it attrs)
- Dim does not really work.
- Can't do headers, lists, paragraphs, footnotes, embeds

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

Testing instructions:

Try the test code on your own or check out
https://nbriannl.github.io/markbind-plain-site/

Proposed commit message: (wrap lines at 72 characters)

Code blocks: Implement inline markdown support for heading

@nbriannl nbriannl marked this pull request as ready for review March 21, 2020 12:59
@nbriannl nbriannl force-pushed the 1119-support-inline-headings branch 2 times, most recently from e77b245 to dead379 Compare March 21, 2020 13:55
Copy link
Contributor

@openorclose openorclose left a comment

Choose a reason for hiding this comment

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

How about some tests, one for a normal heading and one markdown heading?

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

LGTM 👍, just two nits:

</span>
</include>

Headings support inline MarkDown some exceptions (i.e. inline code, dim text style, block quotes, headers, lists, paragraphs, footnotes and embeds).
Copy link
Contributor

Choose a reason for hiding this comment

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

with some exceptions
Could only mention the inline exclusions ( inline code / dim )

+ `<div class="code-block-heading"><span>${heading}</span></div>`
const renderedHeading = markdownIt.renderInline(heading);
const headingStyle = (renderedHeading === heading) ? 'code-block-heading' : 'code-block-heading inline-markdown-heading';
return '<div class="code-block">'
Copy link
Contributor

Choose a reason for hiding this comment

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

indent here should be 2 to the left, is github displaying it wrongly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No you're right. The pains of not having eslint consider this file. 😞

@nbriannl nbriannl force-pushed the 1119-support-inline-headings branch from cde4580 to e880241 Compare March 22, 2020 09:21
@nbriannl nbriannl requested review from ang-zeyu and marvinchin March 22, 2020 09:21
@nbriannl nbriannl force-pushed the 1119-support-inline-headings branch from a3e6484 to 387951f Compare March 23, 2020 03:40
Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

Just wondering - what if the user wants to use symbols that are part of markdown syntax in the heading? (i.e. user wants *Title* rather than Title)

I believe markdown-it should allow escaping with \. Could we confirm that, and perhaps add that to the documentation?

@damithc
Copy link
Contributor

damithc commented Mar 23, 2020

Just wondering - what if the user wants to use symbols that are part of markdown syntax in the heading? (i.e. user wants Title rather than Title)

In fact, that scenario is not limited to headings, but applies to anywhere markdown syntax is used, right?

@marvinchin
Copy link
Contributor

marvinchin commented Mar 23, 2020

In fact, that scenario is not limited to headings, but applies to anywhere markdown syntax is used, right?

Yup, I think that should be the case.

I did some digging around and it seems like we already have a page on escaping markdown - my bad for not noticing it before:
https://markbind.org/userGuide/tipsAndTricks.html#tip-escaping-characters

I think my main concern here would be whether or not introducing this change would lead to "breaking" existing users' code block headings which use characters from the markdown syntax. I don't imagine that there are many existing usages that would fall under this category though, since this is a fairly new feature. Do you think it's necessary for us to mark this as a breaking change?

Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

Found the documentation that I was requesting 👀 LGTM!

@marvinchin marvinchin added this to the v2.12.1 milestone Mar 23, 2020
@nbriannl
Copy link
Contributor Author

Noting conflicts. I'll resolve it asap.

Remove console log and rename var

Update docs

increase line-heigh for headings with inline md

remove line

remove br

update tests

change css structure

update tests

update tests and css class name

Add tests

Address comments

Update docs/userGuide/syntax/code.mbdf

Co-Authored-By: Daryl Tan <[email protected]>
@nbriannl nbriannl force-pushed the 1119-support-inline-headings branch from 387951f to a6337a7 Compare March 24, 2020 05:15
@nbriannl
Copy link
Contributor Author

Rebased on master, ready for merge

@marvinchin marvinchin merged commit e1b292b into MarkBind:master Mar 28, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 28, 2020
…bind into remove-fixed-bugs

* 'remove-fixed-bugs' of https://github.com/Tejas2805/markbind:
  Docs: Add contributing.md (MarkBind#1139)
  Show pointer and use underline dashed for click trigger (MarkBind#1111)
  Support variables to be defined as by JSON (MarkBind#1117)
  Allow an array for specifying page src or glob (MarkBind#1118)
  Code blocks: Implement inline markdown support for heading (MarkBind#1137)
  Fix lazy reload for urls without index (MarkBind#1110)
  Changes remaining references from filterTags to tags (MarkBind#1149)
@nbriannl nbriannl deleted the 1119-support-inline-headings branch April 16, 2020 09:55
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.

code blocks: support inline markdown for heading

5 participants