-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: Introduced Right Subsections for Seamless Topic Access #99
feat: Introduced Right Subsections for Seamless Topic Access #99
Conversation
@aaronbrethorst Sir I have implemented the right subsections. Please review it and suggest if any changes required .. |
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.
please move the code into the specified JS file. otherwise looks great 👍
@aaronbrethorst Done sir. |
frontend/javascript/code_snippets.js
Outdated
@@ -33,6 +33,32 @@ export function copyHeadingDirectLinks() { | |||
}); | |||
} | |||
|
|||
export function setupSidebarItemEventListeners() { |
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.
this needs to move too.
@aaronbrethorst Please review it and suggest if any changes required .. |
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.
I'm going to merge this but I want you to look carefully at the changes that I'm making as an immediate followup to it so you can see what I wanted to see in this PR.
sidebar.style.height = '100%'; | ||
|
||
function appendSidebarItem(textContent, tagName) { | ||
const newItem = document.createElement('p'); |
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.
these should be anchor tags so that they are accessible. That would have the added advantage that you then don't need to apply custom colors on mouseenter and mouseleave.
const h2Elements = document.querySelectorAll('h2'); | ||
const sidebar = document.querySelector('.sidebar'); | ||
|
||
sidebar.style.position = 'fixed'; |
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.
these styles should all be expressed as tailwind classes in the HTML
|
||
export function setupSidebar() { | ||
const h1Elements = document.querySelectorAll('h1'); | ||
const h2Elements = document.querySelectorAll('h2'); |
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.
changing this line to document.querySelectorAll('article h2')
would mean you can get rid of the counting of categories down below. That also future-proofs your changes in case the number of h2 tags in the navigation panel changes. you can't trust that the number of elements on the page will remain fixed. and if you have to for some reason you have to leave a comment explaining your decision
const newItem = document.createElement('p'); | ||
newItem.textContent = textContent; | ||
if (tagName === 'h1') { | ||
newItem.classList.add('sidebar-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.
add()
can take multiple arguments.
Description: Added the appropriate right subsection where all the topics of the pages are listed, allowing users to easily navigate through different topics.
Issue fixed: #75
Changes done:
Screenshots/Videos
✅️ By submitting this PR, I have verified the following