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

[#12222] Instructor help page: Page not scrolling to the right section #12224

Merged
merged 5 commits into from
Mar 23, 2023

Conversation

noreddinelam
Copy link
Contributor

Fixes #12222

Outline of Solution

To resolve this issue, I use to calculate the y coordinate of the section and I used the scrollTo function directly to do it.
The problem was in these two lines :
el.scrollIntoView(); window.scrollBy(0, -50);

The second instruction is not waiting for the first one to finish and do the scroll by -50px.

I removed the '#helpPage' because it is not necessary to get all the children and then search for the section using the id.

@noreddinelam
Copy link
Contributor Author

Still waiting for review

@samuelfangjw
Copy link
Member

Still waiting for review

Open source maintainers on this project are unpaid volunteers with their own work commitments as well. Please be patient on this.

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

Hi @noreddinelam, sorry for the delay! The fix works, but I'm just wondering if it would be better to use a more standard Bootstrap approach. You can refer to this (in particular the .js code)

Edit: Sorry please ignore and refer to @zhaojj2209's reply (:

@zhaojj2209
Copy link
Contributor

TEAMMATES makes use of PageScrollService from the ngx-page-scroll-core library, not sure why this wasn't used here previously. Maybe using this would help to simply things?

@noreddinelam
Copy link
Contributor Author

@samuelfangjw I am sorry. When i saw in the documentation this :
image

i added the comment but of course i understand what u mean.

@noreddinelam
Copy link
Contributor Author

@zhaojj2209 Yeah I noticed that. I tried to use it but unfortunately, It was not working and it was scrolling by like 1cm.

As mentioned here ; (an issue opened in the official project) :
image

The issue link if u want to read all the discussion.

Now, I found a solution and what was the problem. As mentioned in the issue, they said that the scrolling behavior smooth is not working with ngx-page-scroll. The default behavior when we use bootstrap, is smooth and as consequence, all the scrolling mechanism when u touch a link was not working in the application.

Here is the solution :
Set the scroll behavior to auto in the style.scss file like this :

html { scroll-behavior: auto !important; }

And then we can use the ngx-page-scroll.

If u agree with the solution, I can provide a new version of the code.

Feel free to give me more hints.

@zhaojj2209 zhaojj2209 added the s.ToReview The PR is waiting for review(s) label Mar 23, 2023
@zhaojj2209
Copy link
Contributor

@noreddinelam Thank you for investigating this issue! Yes, please proceed with the fix. I suspect this will help to resolve scroll issues elsewhere on the site as well.

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@zhaojj2209 zhaojj2209 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Mar 23, 2023
@noreddinelam
Copy link
Contributor Author

noreddinelam commented Mar 23, 2023

Thanks, for the approval.

Just a question i think that my branch is out-of-date with the master branch so I don't know If I should click on update branch or let you do it when you merge.

@zhaojj2209
Copy link
Contributor

Just a question i think that my branch is out-of-date with the master branch so I don't know If I should click on update branch or let you do it when you merge.

Feel free to update the branch on your end. Maintainers can do it as well (and we do often update contributor PRs before merging), but that usually means our names will show up as PR co-author.

@zhaojj2209 zhaojj2209 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Mar 23, 2023
@noreddinelam
Copy link
Contributor Author

@zhaojj2209 thanks for explanation and the hints given to resolve this issue. I did update the branch.

@zhaojj2209 zhaojj2209 merged commit c0036db into TEAMMATES:master Mar 23, 2023
@zhaojj2209 zhaojj2209 added the c.Bug Bug/defect report label Apr 2, 2023
@zhaojj2209 zhaojj2209 added this to the V8.26.0 milestone Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instructor help page: Page not scrolling to the right section
5 participants