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 dividers and fix bug in siteNav #1063

Merged
merged 19 commits into from
Mar 5, 2020

Conversation

Tejas2805
Copy link
Contributor

@Tejas2805 Tejas2805 commented Feb 27, 2020

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

• [X] Documentation update
• [X] Bug fix

What is the rationale for this request?
Add dividers to siteNav

What changes did you make? (Give an overview)
Added dividers to siteNav and fix bug of input/output mismatch.

Provide some example code that this change will affect:

<code>
* [:house: Home]({{ showBaseUrlText }}/index.html)<br>
---
<br>* Docs<br>
&nbsp;&nbsp;* [User Guide]({{ showBaseUrlText }}/ug.html)<br>
&nbsp;&nbsp;* [Dev Guide]({{ showBaseUrlText }}/dg.html)<br>
---
<br>* [Search]({{ showBaseUrlText }}/search.html)<br>
&nbsp;&nbsp;* [Google Search](https://www.google.com/)<br>
&nbsp;&nbsp;* [YouTube Search](https://www.youtube.com/)<br>
---
<br>* [Contact]({{ showBaseUrlText }}/contact.html)
</code>

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

Testing instructions:
Deployed preview

* '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
@Tejas2805 Tejas2805 changed the title Add dividers in site nav Add dividers and fix bug in site nav Feb 27, 2020
@Tejas2805 Tejas2805 changed the title Add dividers and fix bug in site nav Add dividers and fix bug in siteNav Feb 27, 2020
@nbriannl
Copy link
Contributor

nbriannl commented Feb 28, 2020

Fixes #1064

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.

Thanks for noticing the mistake.
Just a nit, I think it would be better to use MarkBind's code block syntax since we have one now.

As per discussion in #543. I'm not sure whether we should include horizontal dividers in the example code, or show the example code without horizontal dividers and then mention that you can divide via horizontal rule. I think the former is fine, we just have to mention that feature at the 'Step to add a siteNav'. The additional change in bold below.

Steps to add a siteNav:

  1. Format your siteNav as an unordered Markdown list and save it inside the _markbind/navigation directory. You may include dividers in the siteNav via horizontal rules.
  2. Specify the file as the value of the siteNav attribute in the <frontmatter> of the page.

Comment on lines 15 to 28
<code>
* [:house: Home]({{ showBaseUrlText }}/index.html)<br>
* Docs<br>
---
<br>* Docs<br>
&nbsp;&nbsp;* [User Guide]({{ showBaseUrlText }}/ug.html)<br>
&nbsp;&nbsp;* [Dev Guide]({{ showBaseUrlText }}/dg.html)<br>
* [Search]({{ showBaseUrlText }}/search.html)<br>
---
<br>* [Search]({{ showBaseUrlText }}/search.html)<br>
&nbsp;&nbsp;* [Google Search](https://www.google.com/)<br>
&nbsp;&nbsp;* [YouTube Search](https://www.youtube.com/)<br>
* [Contact]({{ showBaseUrlText }}/contact.html)
---
<br>* [Contact]({{ showBaseUrlText }}/contact.html)
</code>
</box>
Copy link
Contributor

Choose a reason for hiding this comment

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

MarkBind now supports code blocks.

```{.no-line-numbers}
* [:house: Home]({{ baseUrl }}/index.html)
* Docs
  * [User Guide]({{ baseUrl }}/ug.html)
  * [Dev Guide]({{ baseUrl }}/dg.html)
* [Search]({{ baseUrl }}/search.html)
  * [Google Search](https://www.google.com/)
  * [YouTube Search](https://www.youtube.com/)
* [Contact]({{ baseUrl }}/contact.html)
```

Could be used instead. Much easier syntax for documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want me to update it to using code blocks ?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be nice. You could also change the other code blocks in this file into MarkBind code blocks, but that would expand the scope of your ticket by abit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. Let me just change this to code block in this PR. Since this PR already affects this. For rest, I will do another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Regarding that other PR, it would be best to wait after #1034 is merged. After that merge, the code blocks will have (almost) full functionality.

Copy link
Contributor Author

@Tejas2805 Tejas2805 Feb 28, 2020

Choose a reason for hiding this comment

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

Clicking on Contact shows Page Not Found. Any suggestion as to what to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Thanks for the changes, just one more change then we're good.

docs/userGuide/syntax/siteNavigationMenus.mbdf Outdated Show resolved Hide resolved
Co-Authored-By: nbriannl <neilbrian.nl@gmail.com>
@Tejas2805 Tejas2805 mentioned this pull request Mar 1, 2020
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.

LGTM

@Tejas2805
Copy link
Contributor Author

@marvinchin Can I get your review on this?

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.

Couple of questions!

@@ -3,26 +3,23 @@
**A _Site Navigation Menu_ (==_siteNav_ for short==) is a fixed menu on the ==left edge== of a page**, that can be used to show a road map of the main pages of your site.

Steps to add a siteNav:
1. Format your siteNav as an unordered Markdown list and save it inside the `_markbind/navigation` directory.
1. Format your siteNav as an unordered Markdown list and save it inside the `_markbind/navigation` directory. You may include dividers in the siteNav via <a target="_blank" href="{{ baseUrl }}/userGuide/readerfacingfeatures#horizontal-rules">horizontal rules</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that we shouldn't open internal links in a new tab. I know that we currently do that in the page and site nav pages, but perhaps we should remove those too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want me to open in same page or remove link altogether?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's open it in the same page.

* [Contact]({{ showBaseUrlText }}/contact.html)
</code>
</box>
```{.no-line-numbers}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think adding a divider in the example would be helpful, since we mentioned it above?

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 think we should. So that they can understand how it looks and accordingly decide whether to use or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't see any changes here?
Screenshot 2020-03-04 at 12 19 05 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah my bad. I was asking you whether to do it or not. Didn't realise you were saying to implement and show. I have done it now. Sorry for the confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, sorry for the lack of clarity!

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.

The horizontal rules look good. Just one nit!

* Docs
* [User Guide]({{ baseUrl }}/ug.html)
* [Dev Guide]({{ baseUrl }}/dg.html)
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit of a nit, but let's not leave trailing spaces behind 🙂

You should also be able to configure your editor to automatically remove trailing whitespace behind, so you don't have to worry about it yourself.

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.11.1 milestone Mar 5, 2020
@marvinchin marvinchin merged commit bf21a10 into MarkBind:master Mar 5, 2020
@yamgent yamgent added the pr.DocsUpdate 📃 Pure changes to the documentation, such as typo, restructuring, etc label Mar 7, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 7, 2020
…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 Tejas2805 mentioned this pull request Mar 7, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 9, 2020
* 'master' of https://github.com/MarkBind/markbind:
  2.12.0
  Update outdated test files
  Update vue-strap version to v2.0.1-markbind.37
  Fix refactor to processDynamicResources (MarkBind#1092)
  Implement lazy page building for markbind serve (MarkBind#1038)
  Add warnings for conflicting/deprecated component attribs (MarkBind#1057)
  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)
@Tejas2805 Tejas2805 deleted the add-divider-in-siteNav branch March 21, 2020 09:20
marvinchin pushed a commit that referenced this pull request Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr.DocsUpdate 📃 Pure changes to the documentation, such as typo, restructuring, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants