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

Convert to code block #1086

Merged
merged 36 commits into from
Mar 16, 2020
Merged

Conversation

Tejas2805
Copy link
Contributor

@Tejas2805 Tejas2805 commented Mar 4, 2020

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

• [x] Documentation update

What is the rationale for this request?
Recommended by @nbriannl in PR #1063

What changes did you make? (Give an overview)
Changed <code> to using code blocks

Provide some example code that this change will affect:
N.A.

Testing instructions:
Can check the preview.

Proposed commit message: (wrap lines at 72 characters)

Let's convert <code> boxes to code blocks  to better showcase the 
feature

Tejas2805 and others added 27 commits February 17, 2020 04:54
* 'master' of https://github.com/MarkBind/markbind:
  Update tests
  Allow using 'none' footer attribute in frontmatter (MarkBind#1002)
  Support line numbers for code blocks (MarkBind#991)
  2.11.0
  Update test files due to changes in PR MarkBind#982
  Update vue-strap version to v2.0.1-markbind.36
  Make highlighting bold (MarkBind#1045)
  Support markdown for header attr in dropdown (MarkBind#1029)
  Add '_site' to the ignored folders in site.json (MarkBind#1046)
  Use path.join instead of string interpolation (MarkBind#1052)
  Implement box markdown header attributes parsing (MarkBind#1025)
  Make the position of top navbar fixed (MarkBind#982)
  Exclude *.md files from being copied over on build (MarkBind#1010)

# Conflicts:
#	docs/css/main.css
Co-Authored-By: nbriannl <neilbrian.nl@gmail.com>
…nvert-to-code-block

* 'master' of https://github.com/MarkBind/markbind:
  Allow changing parameter properties (MarkBind#1075)
  Custom timezone for built-in timestamp (MarkBind#1073)
  Fix reload inconsistency when updating frontmatter (MarkBind#1068)
  Implement an api to ignore content in certain tags (MarkBind#1047)
  Enable AppVeyor CI (MarkBind#1040)
  Add heading and line highlighting to code blocks (MarkBind#1034)
  Add dividers and fix bug in siteNav (MarkBind#1063)
  Fixed navbar no longer covers modals (MarkBind#1070)
  Add copy code-block plugin (MarkBind#1043)
  Render plugins on dynamic resources (MarkBind#1051)
  Documentation for Implement no-* attributes for <box>  (MarkBind#1042)
  Migrate to bootstrap-vue popovers (MarkBind#1033)
  Refactor preprocess and url processing functions (MarkBind#1026)
  Add pageNav to Using Plugins Page (MarkBind#1062)

# Conflicts:
#	docs/userGuide/syntax/siteNavigationMenus.mbdf
@Tejas2805
Copy link
Contributor Author

@nbriannl Can I get your review on this?

@Tejas2805 Tejas2805 marked this pull request as ready for review March 8, 2020 09:19
@Tejas2805
Copy link
Contributor Author

@marvinchin Can I get your review on this? Thank you for your time.

@Tejas2805
Copy link
Contributor Author

@nbriannl Can I get your review on this?

Copy link
Contributor

@nbriannl nbriannl left a comment

Choose a reason for hiding this comment

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

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.

Please also link the issue you are fixing in the PR description. This allows github to automatically close the issue when this is merged.

docs/userGuide/syntax/pageHead.mbdf Show resolved Hide resolved
{{ mainContentBody }}
```html
```html {heading="page.md", highlight-lines="1"}
MAIN_CONTENT_BODY
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a variable. (previously this was {{ mainContentBody }})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I am putting is a variable this is how it gets rendered

Screen Shot 2020-03-12 at 6 02 15 PM

How do you want me to implement it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we would need to be able to escape the variable in the code block in order to render it as is rather than replace it with the actual value.

There is a PR that aims to address this (#1049) but I haven't had the time to review that yet.

Alternatively, we could do a dirty hack and use the zero width space to break up the variable syntax when declaring the mainContentBody variable:

<variable name="mainContentBody">
{&#8203;{ MAIN_CONTENT_BODY }}
</variable>

If we do this, we should leave a todo reminding us to change it to use the escape tags once that PR is ready.

I'd lean towards the latter, so we can make a clean move towards code blocks and avoid any potential merge conflicts if there are any new changes to the documentation, since I'm not sure when that PR is going to be merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay will do accordingly.

{{ mainContentBody }}
```html
```html {heading="page.md"}
<variable name="mainContentBody">
Copy link
Contributor

Choose a reason for hiding this comment

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

No, I didn't mean to insert this here 😅 If you look at the top of this file, there is a variable declared for mainContentBody, and the value is substituted whenever we use the variable by writing {{ mainContentBody }}. We should change the value of the variable, and continue to use the variable here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah sorry. Didn't realise there was a variable declared on top of the file. Thanks for guiding.

Also, can you help explain what does &#8203 do?

Copy link
Contributor

@marvinchin marvinchin Mar 13, 2020

Choose a reason for hiding this comment

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

It's the unicode for zero width space. This allows us to break up the {{ variable }} syntax and avoid markbind from treating it as a variable.

https://www.fileformat.info/info/unicode/char/200b/index.htm

Let's also leave a comment explaining why we are doing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also leave a TODO to remind us to change to a proper escape tag once #1049 is ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

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.

Sorry, found an issue with the zero width space hack that I suggested. Let's revert the changes for that file for now. We should be good to go once we have changed that.

I think this also presents a compelling use case for #1049.

<variable name="mainContentBody">
<code>{<span></span>{ MAIN_CONTENT_BODY }}</code>
{&#8203;{ MAIN_CONTENT_BODY }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I didn't realise this earlier, but the zero width space is preserved when the user copies the text from the page. This would cause the snippet to not work if the user copies and pastes it into their own editor.

Let's revert back to the original version for the segments that require this variable, and leave a todo for us to change it once the escape tags PR is ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. We should be good to go, I guess.

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.

LGTM

@marvinchin marvinchin added this to the v2.12.1 milestone Mar 16, 2020
@marvinchin marvinchin merged commit bdd1f60 into MarkBind:master Mar 16, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 21, 2020
@Tejas2805 Tejas2805 deleted the convert-to-code-block branch March 21, 2020 09:19
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 23, 2020
…x-pageNav

* 'master' of https://github.com/MarkBind/markbind:
  Update test files (MarkBind#1138)
  Remove OK button from modals (MarkBind#1134)
  Add start from line number functionality to code blocks (MarkBind#1115)
  Allow special tags to be self-closing (MarkBind#1101)
  Simplify baseUrl resolving process (MarkBind#1087)
  Remove redundant file writing (MarkBind#1090)
  Bump acorn from 5.7.3 to 5.7.4 (MarkBind#1121)
  Bump acorn from 7.1.0 to 7.1.1 in /src/lib/markbind (MarkBind#1120)
  Unify markdown-it parser variants (MarkBind#1056)
  Remove dynamic include feature (MarkBind#1037)
  Fix flex shrink not applying in content wrapper (MarkBind#1135)
  Escape Nunjucks for special tags (MarkBind#1049)
  Update documentation on icon slot for boxes (MarkBind#1123)
  Convert code in boxes to code block (MarkBind#1086)
marvinchin pushed a commit that referenced this pull request Apr 10, 2020
Let's convert <code> boxes to code blocks to better showcase the 
feature
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

3 participants