[#12010] Notifications box overlaps with the page menu in the instructor help page

[techMedMau]

Fixes #12010

＊ add margin-top attribute to .side-nav class

[domlimm]

I believe the page menu should still be at the top right and not pushed downwards using a hardcoded margin-top. We should try to stick with the CSS properties that bootstrap has provided.

This is what I currently see at my end (locally):

Also, I have tried it on production and it seems as though this happens when the notification container is present (image below as per the one in the issue description):

However, after removing the notification by hitting on X, it looks fine.

@jasonqiu212 @weiquu Any thoughts on this? And how Maureen can reproduce it as a comment in the issue mentioned that only registered users get the notifications.

[techMedMau]

Hi, I would try another way to fix it. Just to clarify one thing. I use the admin account to announce a notification to reproduce the issue. Is this the correct way to do it?

[domlimm]

@techMedMau You're right! Yes we navigate to the Notifications page as an admin and add a notification from there:

[techMedMau]

Change in latest commit

＊Detect current URL in notification banner component. If the URL contains "instructor/help" bind bootstrap col-lg-10 class to the banner component

[domlimm]

Looks good! In a way, we're resizing the notification banner to follow the size of the Help for Instructors section, that is specific to this page .../web/instructor/help. Not sure how scalable it is in the future. Have checked other pages as an Instructor and Student, nothing odd spotted.

Let's see what other reviewers think.

To note, currently in prod, the notification banner stretches to take up the space as much as it is able to of the portion of the page that it resides in (as the below image).

And for comparison, do look at the latest image posted by Maureen.

[jasonqiu212]

Personally, I think this fix is not very scalable in the future, since we are customizing the behavior of the notification banner for a specific web page. Since the problem occurs with the fixed positioning of the menu, I think it would be cleaner to find a fix with the menu, rather than modifying the notification banner.

One solution I tried - Have the menu underneath the notification banner and use sticky positioning, instead of fixed

Fix.mov

Outline of solution:

• Use position: sticky instead
• Place the main column and side menu into a row flexbox

But, there are downsides to using position: sticky, namely that Internet Explorer does not support it. IE will default to position: relative, meaning that the side menu will not fix in place when the user scrolls the page.

@domlimm What do you think about this alternative?

[zhaojj2209]

One solution I tried - Have the menu underneath the notification banner and use sticky positioning, instead of fixed

This reminded me - the notification banner was originally designed to be sticky, but it was removed due to a bug on Safari 14 at the time (see #11747). The bug seems to have been fixed since then, so let's reintroduce position: sticky.

[techMedMau]

One solution I tried - Have the menu underneath the notification banner and use sticky positioning, instead of fixed

This reminded me - the notification banner was originally designed to be sticky, but it was removed due to a bug on Safari 14 at the time (see #11747). The bug seems to have been fixed since then, so let's reintroduce position: sticky.

Hi,
@zhaojj2209
I tried the sticky on notification banner, but it hides the menu if users do not close it. Is that how it suppose to look like as video below?

Screen.Recording.2023-05-21.at.3.49.55.PM.mov

Besides, since @jasonqiu212 fix it in another way, do I need to do the same thing as him and do the commit?

[domlimm]

@techMedMau Hey Maureen, I believe it should be as how @jasonqiu212 has shown it! Thanks for making the changes!

[jasonqiu212]

For your reference @techMedMau, here's what I did in my attempts:

instructor-help-page.component.scss:

.side-nav {
  position: -webkit-sticky; /* for Safari */
  position: sticky;
  align-self: flex-start;
  top: 100px;
}

instructor-help-page.component.html:

• Add everything under a new row <div> component

<div class="row">
...
</div>

Hope it helps!

[techMedMau]

Sorry for asking question about testing.
I always can not pass the E2E tests.
I looked into the details, but I don't think I modify those component which make the test failed.
Besides, there was once, my PR didn't pass the E2E test; but when I came back and took the next day, it passed.
Did any of you make any modification? I'd like to know every details.
Thank you!

[cedricongjh]

Sorry for asking question about testing. I always can not pass the E2E tests. I looked into the details, but I don't think I modify those component which make the test failed. Besides, there was once, my PR didn't pass the E2E test; but when I came back and took the next day, it passed. Did any of you make any modification? I'd like to know every details. Thank you!

Hi @techMedMau, the reason is that unfortunately the E2E tests are unstable, and through no fault of yours. The instance you described is likely due to one of our maintainers re-running the test again, and thus causing it to pass when you looked at it the next day.

[techMedMau]

Change of latest commit
＊ apply sticky on menu & add row to both main and menu
＊ I didn't appy -webkit-sticky. Since without this, it still work well in safari.

In Safari

Screen.Recording.2023-05-22.at.10.44.25.AM.mov

In Chrome

Screen.Recording.2023-05-22.at.10.44.58.AM.mov

[kevin9foong]

hi @domlimm , @weiquu, any updates on the review?

[domlimm]

Other than the nit, LGTM! Thank you for making the changes, and your contribution Maureen! 👍🏻

[weiquu]

LGTM! Thank you for contributing (: