Skip to content
This repository has been archived by the owner on Jul 12, 2020. It is now read-only.

Panel: Change header markdown rendering to non-inline #81

Merged
merged 2 commits into from
Jul 20, 2018

Conversation

yamgent
Copy link
Member

@yamgent yamgent commented Jul 20, 2018

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?
Sometimes, panel uses Markdown headings. However, due to changes in #74, the Markdown rendering technique used is inline, and inline text normally do not use headings, so they are not parsed, and as a result they are broken.

What changes did you make? (Give an overview)

  • Switch the rendering from inline to non-inline.
  • Fix the seamless panel bug, which was the reason why it was changed to inline in the first place.

Provide some example code that this change will affect:

<panel header="# Some headings">
</panel>
Before After

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

Testing instructions:

  1. Build bootstrap, copy vue-strap.min.js to MarkBind repository.
  2. Run markbind serve docs and ensure that no panels are broken in Component Reference.
  3. Also try the following index.md:
<panel type="seamless" header="Normal">
</panel>

<panel type="seamless" header="# Heading 1">
</panel>

<panel type="seamless" header="## Heading 2">
</panel>

<panel type="seamless" header="### Heading 3">
</panel>

<panel type="seamless" header="#### Heading 4">
</panel>

<panel type="seamless" header="##### Heading 5">
</panel>

<panel type="seamless" header="###### Heading 6">
</panel>

<panel header="Normal panel">
</panel>

<panel header="# Normal panel 2">
</panel>

<panel type="seamless">
  <p slot="header" class="card-title">
    <i><strong>
      <span style="color:#FF0000;">R  </span>
      <span style="color:#FF7F00;">A  </span>
      <span style="color:#FFFF00;">I  </span>
      <span style="color:#00FF00;">N  </span>
      <span style="color:#0000FF;">B  </span>
      <span style="color:#4B0082;">O  </span>
      <span style="color:#9400D3;">W  </span>
      Custom slot
    </strong></i>
  </p>
</panel>

renderInline() is used when trying to render markdown syntax surrounded
by other normal text.

Some Markdown syntax are not supported inline. For example, one will not
expect a "header" to appear in the middle of other text, so Markdown
will not parse headers even if it is correctly written.

Change calls of md.renderInline() to md.render() for header rendering.
@yamgent
Copy link
Member Author

yamgent commented Jul 20, 2018

Update:

  • Ready for review.

@Chng-Zhi-Xuan
Copy link

Chng-Zhi-Xuan commented Jul 20, 2018

Algorithm for inserting the chevron HTML within wrapped headerContent

Edit1: changed isHeaderTag regex

function isHeaderTag(s) {
  if (/\B(<h[1-6]{1}>)\B/g.test(s)) {
    return 4;
  }
  return 0;
}

const chevron = '<span class="something"></span>' + ' ';
const mdRenderedText = '<h1>hello <h1>hello</h1> <mark>dude</mark></h1>';
const tagRegex = /<[a-z1-6]*>/g;

const splitArray = mdRenderedText.replace(tagRegex, '$& ').split(' ');
const lastIndex = isHeaderTag(splitArray[0]);

let result = '';

if (lastIndex !== 0) {
  result = mdRenderedText.slice(0, lastIndex) + chevron + mdRenderedText.slice(lastIndex);
} else {
  result = chevron + mdRenderedText;
}

console.log(result);

@yamgent
Copy link
Member Author

yamgent commented Jul 20, 2018

Update:

  • Fixed caret placement for seamless.
  • Updated original post to include another test case (see step 3 of "Testing Instructions").

@Chng-Zhi-Xuan
Copy link

Chng-Zhi-Xuan commented Jul 20, 2018

Nice 👍, caret remains inline and is able to toggle correctly.

However, I found a style problem:
81-panel-header

The default Bootstrap line-height of 1.2 for headers is too low and causes highlighted text to block text above it.

Recommend to override CSS for div.card-title > h1 to h6 { ... }

@damithc
Copy link

damithc commented Jul 20, 2018

The default Bootstrap line-height of 1.2 for headers is too low and causes highlighted text to block text above it.

Can we reduce the height of highlighting?

@yamgent
Copy link
Member Author

yamgent commented Jul 20, 2018

Can we reduce the height of highlighting?

We can try that, in fact that will be the best solution.

However, the highlighting issue is already present even before the presence of the seamless bug, so it should be solved in a separate PR, not here.

Will be merging this PR today before the release, because this is a critical PR, seamless panel header rendering will break otherwise.

@yamgent yamgent added this to the v2.0.1-markbind.15 milestone Jul 20, 2018
@damithc
Copy link

damithc commented Jul 20, 2018

However, the highlighting issue is already present even before the presence of the seamless bug, so it should be solved in a separate PR, not here.

Will be merging this PR today before the release, because this is a critical PR, seamless panel header rendering will break otherwise.

Sounds good.

When the panel is seamless, if the contents of the header is wrapped by
a paragraph HTML tag or header HTML tag, the header will be on the next
line after the caret, rather than be on the same line with the caret:

>
Header content

Therefore, we must manually insert the caret into the header content in
order for everything to stay in the same line, like this:

> Header content
@yamgent yamgent force-pushed the fix-panel-heading branch from 97225e0 to 43f969f Compare July 20, 2018 07:03
@yamgent yamgent merged commit 6b87950 into MarkBind:master Jul 20, 2018
@yamgent yamgent deleted the fix-panel-heading branch July 20, 2018 07:06
yamgent added a commit to yamgent/vue-strap that referenced this pull request Jul 27, 2018
yamgent added a commit to yamgent/vue-strap that referenced this pull request Jul 27, 2018
The caret is combined with the header content. It is usually rendered
like this:

> Some heading

However, custom header contents, after markdown rendering, will be
wrapped with block HTML tags such as <p> and <h1>, which will make
browsers render a line break between the caret and header content:

>
Some heading

Let's fix this by moving the caret to a separate div. Then, make
everything inside a panel header be on a single line by using CSS
'display: inline-block' and 'width: ...', such that the width adds up to
100%.

    +--------+-----------------+---------+
    | caret  |  header-content | buttons |
    | (32px) | (100% - 32 - 32)|  (32px) |
    +--------+-----------------+---------+

Thanks to @nusjzx for providing the fix to the button-wrapper in PR
MarkBind#86, in which his use of
'calc(100% - 32px)' provided a source of inspiration for this fix. His
fix in that PR is incorporated into this commit because without it, the
responsive design will not work.

An alternative fix considered was to use md.renderInline(). However,
md.renderInline() does not render markdown headings.

Another alternative fix was to insert the caret directly inside the
header content. However, after PR MarkBind#74, MarkBind#81 and MarkBind#83 were merged, new
problems continue to emerge.

These problems are reported as comments in these pages:
- MarkBind#74
- MarkBind#81
- MarkBind#83
- MarkBind/markbind#373
- MarkBind/markbind#383

As such, the PRs for this alternative fix has been reverted by the
previous commits before this commit. Therefore, this commit shall be
viewed as the canonical fix for issue
MarkBind/markbind#337.
yamgent added a commit to yamgent/vue-strap that referenced this pull request Jul 27, 2018
The caret is combined with the header content. It is usually rendered
like this:

> Some heading

However, custom header contents, after markdown rendering, will be
wrapped with block HTML tags such as <p> and <h1>, which will make
browsers render a line break between the caret and header content:

>
Some heading

Let's fix this by moving the caret to a separate div. Then, make
everything inside a panel header be on a single line by using CSS
'display: inline-block' and 'width: ...', such that the width adds up to
100%.

    +--------+-----------------+---------+
    | caret  |  header-content | buttons |
    | (32px) | (100% - 32 - 32)|  (32px) |
    +--------+-----------------+---------+

Thanks to @nusjzx for providing the fix to the button-wrapper in PR
MarkBind#86, in which his use of
'calc(100% - 32px)' provided a source of inspiration for this fix. His
fix in that PR is incorporated into this commit because without it, the
responsive design will not work.

An alternative fix considered was to use md.renderInline(). However,
md.renderInline() does not render markdown headings.

Another alternative fix was to insert the caret directly inside the
header content. However, after PR MarkBind#74, MarkBind#81 and MarkBind#83 were merged, new
problems continue to emerge.

These problems are reported as comments in these pages:
- MarkBind#74
- MarkBind#81
- MarkBind#83
- MarkBind/markbind#373
- MarkBind/markbind#383

As such, the PRs for this alternative fix has been reverted by the
previous commits before this commit.
yamgent added a commit to yamgent/vue-strap that referenced this pull request Jul 27, 2018
The caret is combined with the header content. It is usually rendered
like this:

> Some heading

However, custom header contents, after markdown rendering, will be
wrapped with block HTML tags such as <p> and <h1>, which will make
browsers render a line break between the caret and header content:

>
Some heading

Let's fix this by moving the caret to a separate div. Then, make
everything inside a panel header be on a single line by using CSS
'display: inline-block' and 'width: ...', such that the width adds up to
100%.

    +--------+-----------------+---------+
    | caret  |  header-content | buttons |
    | (32px) | (100% - 32 - 32)|  (32px) |
    +--------+-----------------+---------+

Thanks to @nusjzx for providing the fix to the button-wrapper in PR
MarkBind#86, in which his use of
'calc(100% - 32px)' provided a source of inspiration for this fix. His
fix in that PR is incorporated into this commit because without it, the
responsive design will not work.

An alternative fix considered was to use md.renderInline(). However,
md.renderInline() does not render markdown headings.

Another alternative fix was to insert the caret directly inside the
header content. However, after PR MarkBind#74, MarkBind#81 and MarkBind#83 were merged, new
problems continue to emerge.

These problems are reported as comments in these pages:
- MarkBind#74
- MarkBind#81
- MarkBind#83
- MarkBind/markbind#373
- MarkBind/markbind#383

As such, the PRs for this alternative fix has been reverted by the
previous commits before this commit.
@yamgent yamgent mentioned this pull request Jul 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants