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

WIP fix: Allow pop up notifications to go under the navigation when scrolling #14222

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

petesfrench
Copy link
Contributor

@petesfrench petesfrench commented Aug 22, 2024

Done

  • Lower the z-index of notifications so they go under the navigation
  • Put the notifications in their own file

QA

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-11728

@webteam-app
Copy link

@anthonydillon
Copy link
Contributor

Thank you, this definitely works but the scroll brings the navigation now over the notification. Could you try adding scroll-padding-top to the notifications to try and block this behaviour?

@pastelcyborg pastelcyborg self-requested a review September 4, 2024 19:14
@pastelcyborg
Copy link
Contributor

@petesfrench Would you mind updating the description of this to explain exactly what all these changes are? This seems like a large number of modifications for something that reads as simply needing a z-index tweak, and I'm not sure why all these changes were made.

@petesfrench petesfrench changed the title fix: Lower the z-index of notifications so they go under the navigation WIP fix: Lower the z-index of notifications so they go under the navigation Sep 17, 2024
@petesfrench petesfrench changed the title WIP fix: Lower the z-index of notifications so they go under the navigation WIP fix: Lower pop up notifications to go under the navigation when scrolling Sep 17, 2024
@petesfrench
Copy link
Contributor Author

@pastelcyborg Sure thing. Note that the majority of the changes come from formatting changes applied by Jinja. The scope of the pr has not changed.

I got a little stuck on this and haven't had time to jump back into it. If you want to look into it and see if you can get it working feel free

@petesfrench petesfrench changed the title WIP fix: Lower pop up notifications to go under the navigation when scrolling WIP fix: Allow pop up notifications to go under the navigation when scrolling Sep 17, 2024
@pastelcyborg
Copy link
Contributor

pastelcyborg commented Sep 18, 2024

After looking into this a bit further, I think my inclination here would actually be to make the #newsletter-signup (and probably other similar confirmation notifications) fixed position, rather than do any z-index manipulation. Not only would this avoid any placement issues, since the notification is already located vertically lower than the navigation in the DOM, it would also be a slightly improved UX, since these notifications should really be visible to the user regardless of their scroll position anyway.

@petesfrench Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants