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

Show mobile navigation menus only if links are present #1561

Merged
merged 1 commit into from
May 4, 2021

Conversation

ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented May 2, 2021

What is the purpose of this pull request?

Overview of changes:

  • Change the checks for mobile navigation menus to require links to be present
  • Flourish the documentation a little more so this note is clearer

Anything you'd like to highlight / discuss:
na

Testing instructions:

8 cases, all testable in the formattingContents.md / any other page with both site + page nav:
(also in snapshot tests)

// Edit layouts/userGuide.md to have:

// Case 1 & 2
// **with** and **without** wrapping id="site-nav"
// mobile nav menu button does not show up
<site-nav>
no links here
</site-nav>

// Cases 3 & 4
// Case 3 (with) also testable with a simple <a /> tag
// **with** and **without** wrapping id="site-nav"
// mobile nav menu button shows up
<site-nav>
* [**Getting Started**]({{baseUrl}}/userGuide/gettingStarted.html)
</site-nav>

// Cases 5 & 6
// Case 5 (with) also testable with a simple <a /> tag
// **with** and **without** wrapping id="page-nav"
// mobile page nav button shows up
{{ pageNav }}

// Cases 7 & 8
// Case 
// **with** and **without** wrapping id="page-nav"
// mobile page nav button does not show up
{{ pageNav }} // however, erase all headings in formattingContents.md

Proposed commit message: (wrap lines at 72 characters)
Hide mobile navigation menus if there are no links


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Nice work 👍 I've tried out the changes and everything seems to be working. LGTM :)

@jonahtanjz jonahtanjz merged commit d45442b into MarkBind:master May 4, 2021
@jonahtanjz jonahtanjz added this to the v3.0 milestone May 6, 2021
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.

2 participants