-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
refactor: remove JQuery from parts of Search JS #2970
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
Conversation
✅ Deploy Preview for hugo-portfolio-theme canceled.
|
✅ Deploy Preview for academic-demo canceled.
|
✅ Deploy Preview for markdown-slides canceled.
|
document.querySelector('#TableOfContents').classList.add('nav flex-column'); | ||
} | ||
if (document.querySelector('#TableOfContents li')) { | ||
document.querySelector('#TableOfContents li').classList.add('nav-item'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be multiple <li>
, so surely this requires a foreach statement?
Similar feedback applies to the other instances where the query selector matches multiple elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested all these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Konafets I see you have pushed some more changes but have not responded to the above comments yet?
Do the new commits fix all the issues and have you now tested that the PR works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
yes after your question I get back and tested all changes successfully. This means, multiple tables are handled correctly, the table of content and the checkboxes get the bullets removed. From my tests, it works like it did before.
bc6ff79
to
02083a9
Compare
Towards achieving #1402