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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

h3 topics with Markdown formatting causes a glitch on mobile #1254

Closed
Luanf opened this issue Mar 3, 2019 · 4 comments

Comments

Projects
None yet
4 participants
@Luanf
Copy link

commented Mar 3, 2019

馃悰 Bug Report

Found this while investigating a Jest issue: facebook/jest#7856
When navigating a "topics" (?) menu on a mobile screen size, clicking an h3 header that has Markdown formatting does not closes the menu.

Have you read the Contributing Guidelines on issues?

Yes.

To Reproduce

Note: I'm using a local Jest repository using Docusaurus 1.7.2 to reproduce this

  1. Add some Markdown formatting (such as bold, italics or code) to a h3 header on a document.

  2. Use a responsive website size small enough to have this UI:
    issue_23_first

  3. Click the vertical three dots button to see all references:
    issue_23_second

  4. Attempting to navigate to a h3 section formatted with Markdown will not hide the page, although it correctly redirects the URL:
    issue_23_third

Expected behavior

The open menu should have closed.

Actual Behavior

URL changes but the menu isn't closed.
Removing the backticks from the h3 makes it function properly.
Adding other formatting such as bold and italics generate the same issue.

@shakcho

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

I'd like to help with this issue, as I'm first-time contributor and the issue is labeled with good-first-time, is there anything specific thing I need to take care of, please let me know.
Thanks

@shakcho

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

I've found the issue, basically in /Docusaurus/packages/docusaurus-1.x/lib/core/nav/SideNav.js in line number 190

const headings = document.querySelector('.toc-headings');
headings && headings.addEventListener('click', function(event) {
                if (event.target.tagName === 'A') {
                  document.body.classList.remove('tocActive');
                }
  }, false);

we are adding click listener on ".toc-headings" and removing toActive class if the target is "A" tag, where in case of Jest documention it is

<li><a href="#afterallfn-timeout"><code>afterAll(fn, timeout)</code></a></li>

so the target will come as "CODE" tag instead of "A" tag, hence it is breaking.

Proposed Solution

const headingsLink = document.querySelectorAll('.toc-headings a');
for (let i = 0; i < headingsLink.length;  i++) {
      headingsLink[i].addEventListener( "click", function(event) {
                document.body.classList.remove('tocActive');
  }, false);
@yangshun

This comment has been minimized.

Copy link
Member

commented May 4, 2019

Thanks for looking into this! I'm not sure if the proposed solution works, but I think there's a better way than adding event listeners to all the <a> as there could be a performance overhead of adding too many event listeners. The original approach tries to overcome this overhead by using event delegation but its implementation failed to handle the case of other elements being targets. This problem also exists on docusaurus.io: https://docusaurus.io/docs/en/commands#docusaurus-publish

I think the proper fix is to recursively traverse the parents of the event.target with event.target.parentNode and terminate when:

  • an instances where tagName === 'A' is found
  • It hits .toc-headings (means it's not clicking on any <a> or its children)

Feel free to create a PR to fix this! You can use the local Docusaurus site to test the fix.

@shakcho

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

Hi @yangshun I actually thought about the performance issue with the above solution and thought to do it in the same approach you mentioned, but I thought to traverse to the parents will be more costly.

Anyway, I would like to propose another solution which is pure CSS fix and no code changes will be required in the js.

The CSS only Solution

.toc-headings a { 
    position: relative;
}
.toc-headings a:after { 
  position : absolute ;
  content : "";
  left : 0;
  right : 0;
  top : 0;
  bottom: 0;
}

By using this css style, we basically force all the click event in Anchor tag, it doesn't matter whatever the children present.

Your proposed solution with JavaScript

const headings = document.querySelector('.toc-headings');
headings && headings.addEventListener('click', function(event) {
  let el = event.target;
  while(el !== event.currentTarget){
    if (el.tagName === 'A') {
      document.body.classList.remove('tocActive');
      break;
    } else{
      el = el.parentNode;
    }
  }
}, false);

I've tested both the solution and both are working fine.
We can choose any one of them, please let me know which one will better way to solve the issue also let me know if I'm missing something, Based on that I will create the Pull Request.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.