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

add attributes to fence blocks #2394

Merged
merged 15 commits into from
Nov 6, 2018
Merged

add attributes to fence blocks #2394

merged 15 commits into from
Nov 6, 2018

Conversation

daiyam
Copy link
Contributor

@daiyam daiyam commented Sep 13, 2018

This change adds attributes to fence blocks like: mermaid(50h):Graph 1

Its format is: language(attributes):fileName:firstLineNumber.
The formats for an attribute are: name="value" or name='value' or name=value or name.
The attribute <number>h is transformed as height=<number>

For example:

  • javacript:test.js:5
  • mermaid
  • mermaid(50h)
  • mermaid(height=50):Graph 1
  • mermaid(height="50"):Graph 1
  • mermaid(height='50')
  • chart(yaml 50h):My chart 8

chart, flowchart, mermaid, sequence will display the fileName as title.

This is a better fix for #2335 than my previous proposed pull request #2340

screenshot

@daiyam daiyam mentioned this pull request Sep 13, 2018
@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Sep 14, 2018
alt: ['paragraph', 'reference', 'blockquote', 'list']
})

for (let name in renderers) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use const instead of let

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZeroX-DG thanks! I did learn something today The for-of loop

Copy link
Member

Choose a reason for hiding this comment

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

Haha, amazing right? I also discorvered the for-of loop a few months ago 😄 I realized that I've been in stone age for years 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did know the for-of loop but I didn't know I could use const for the iterator. (Since the loop do change the value/address of the variable)

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Your code re-introduce the old XSS bug. Please fix it. (Here is @Rokt33r fix for that bug: #2191)

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Sep 15, 2018
@daiyam
Copy link
Contributor Author

daiyam commented Sep 15, 2018

@ZeroX-DG I've pushed the fix

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

@daiyam Please change your code to my request. Also, can you change the code in MermaidRender.js in the catch block in render function? There's a console.error command there and it keep throwing error while I typing the graph and we don't need that.

&>span.filename
width 100%
border-radius: 5px 0px 0px 0px
margin -8px 100% 8px -8px
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change to fix a small display bug:
z-index: 10
Before:
image
After:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ZeroX-DG I removed the errors for every diagram (chart, flowchart, mermaid and sequence).
I've also fixed a bug with the line number...

But I can't replicated the visual bug you are getting...
Here what I get:
screenshot

Here my preferences:
screenshot

Copy link
Member

Choose a reason for hiding this comment

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

You preview font size is too small, you can set it to 24 and you will see the problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here my fix:
screenshot

@ZeroX-DG
Copy link
Member

@daiyam Can you fix the conflict ?

@daiyam
Copy link
Contributor Author

daiyam commented Oct 14, 2018

@ZeroX-DG it's done

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Oct 14, 2018
@Rokt33r
Copy link
Member

Rokt33r commented Nov 6, 2018

@daiyam I'm sorry for being late response. Could you fix the conflict?

@Rokt33r Rokt33r removed the approved 👍 Pull request has been approved by sufficient reviewers. label Nov 6, 2018
@daiyam
Copy link
Contributor Author

daiyam commented Nov 6, 2018

@Rokt33r all done

@Rokt33r
Copy link
Member

Rokt33r commented Nov 6, 2018

Awesome!

@Rokt33r Rokt33r merged commit 3793378 into BoostIO:master Nov 6, 2018
@daiyam daiyam deleted the fence-attrs branch November 6, 2018 10:09
@100ideas
Copy link

100ideas commented Nov 7, 2018

I've tried the recently released v0.11.10 and also cloned the repo and built the latest master branch, and in both cases I am noticing that mermaid gantt charts seem to be scaling in such that they are rendered at a higher resolution then shrunk to fit my window, or they are too tall.

  1. The mermaid(40h):graph2 height syntax in the latest build worked for me and helps a lot - thanks @daiyam!

    1. Question: if we are setting the height of the mermaid svg via 'vh' units AFTER the svg is rendered, perhaps mermaid first renders the graph at the full width of the screen, then scales it down proportional to vh? that might account for what I was seeing in the master build.
  2. it looks like mermaid gantt charts have an undocumented config option useWidth: <px> - see mermaid/blob/master/src/diagrams/gantt/ganttRenderer.js#L35 source code and try it in the online editor. 1) Mermaid renders the chart at the width specified by this config option if it is provided; 2) otherwise it uses the width of the containing dom element; 3) otherwise it defaults to useWidth=1200.

Here is where useWidth is set in gattRenderer.js:
https://github.com/knsv/mermaid/blob/master/src/diagrams/gantt/ganttRenderer.js#L35

export const draw = function (text, id) {
  parser.yy.clear()
  parser.parse(text)

  const elem = document.getElementById(id)
  w = elem.parentElement.offsetWidth

  if (typeof w === 'undefined') {
    w = 1200
  }

  if (typeof conf.useWidth !== 'undefined') {
    w = conf.useWidth
  }

I've been playing around with it by hardcoding useWidth: 800 in Boostnote/browser/components/render/MermaidRender.js#29:

// line 29
mermaidAPI.initialize({
  theme: isDarkTheme ? 'dark' : 'default',
  themeCSS: isDarkTheme ? darkThemeStyling : '',
  gantt: { "useWidth": 800}
  // useMaxWidth: false  // <-- I don't think this maps to a valid mermaid config option;
                         //     should be `sequence: {useMaxWidth: false},`
                         //     https://github.com/knsv/mermaid/blob/master/src/mermaidAPI.js#L156
})

boostnote-master developbuild gantt

If others want to set a minimum width, maybe the same result could be obtained more cleanly by configuring min-width of the parent element of mermaid's svg in the boostnote theme; it has class .mermaid: <pre class="mermaid">... In theory the mermaid gantt renderer would set width based by picking up the parent element's offsetWidth?

If you want to experiment with this useWidth config option, you can try the mermaid live editor:

default theme

mermaid-gantt-default-liveeditor

config.gantt.useWidth:"600"

mermaid-gantt-usewidth-liveeditor


for posterity - this is what the current build (w/o the fixes made earlier in this PR) looks like
boostnote-v0 10 11 release gantt

@daiyam
Copy link
Contributor Author

daiyam commented Nov 8, 2018

@100ideas the 1200 is just a fallback if there is no width (in a display:none).
But the problem is that it's using the width of the window as its width.

@daiyam
Copy link
Contributor Author

daiyam commented Nov 8, 2018

@100ideas I've found a way to fix this. You can test it with the PR #2585.

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.

None yet

4 participants