-
Notifications
You must be signed in to change notification settings - Fork 232
Doc right section fix #6465
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
base: master
Are you sure you want to change the base?
Doc right section fix #6465
Conversation
ad9c07d to
6e5a7d4
Compare
|
@eeshaanSA Please take a look. |
docs/assets/js/toc-scroll-spy.js
Outdated
| activeSection.link.style.fontWeight = 'bold'; | ||
| activeSection.link.style.color = '#007bff'; // Bootstrap primary color |
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.
@antedotee, wouldn't it be better to let JS only toggle the active class and keep all styling in SCSS to avoid duplication and overrides, what do you think?
animation_right_side.mp4@eeshaanSA As mentioned in the previous meeting, you suggested that anyone with JS experience to take a look at this. I have reviewed and tested it locally. Overall the behavior looks good. A couple of suggestions:
Please address the review comment about removing inline styles from JS. Once those are fixed, this LGTM. |
6e5a7d4 to
00bfef9
Compare
Signed-off-by: antedotee <soniyadav2051982@gmail.com>
Signed-off-by: antedotee <soniyadav2051982@gmail.com>
… shift by reserving space for inactive links Signed-off-by: antedotee <soniyadav2051982@gmail.com>
00bfef9 to
8aff33e
Compare
|
@rahulshendre I have implemented the suggested changes. Please have a look and let me know if there are any further improvements to be worked upon. |
|
@antedotee I pulled the latest changes and tested them locally. The current behaviour still feels off in practice. |
What this PR does:
This PR adds the functionality of highlighting of the right section along with scrolling of the main content.
Why we need it:
To enhance the user experience
Fixes #6464