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

Notifications panel scrolling doesn't work on high-resolution displays #120

Open
matteocontrini opened this issue Mar 22, 2021 · 6 comments
Assignees
Labels

Comments

@matteocontrini
Copy link

I have a report that the scrolling of the notifications panel is still broken, and it appears to be related to the fact that the screen has a 4K resolution. The problem appears on multiple browsers. I'm told that resizing the window to make it smaller fixes the issue.

Video follows.

2021-03-22.08-11-58.mp4
@davwheat
Copy link
Member

I think it'd be a good idea to rewrite the drop-down/page to remove this issue once and for all.

My suggestion would be to load an amount of notifications based on the height of the viewport initially, or to limit the dropdown height with CSS.

@askvortsov1
Copy link
Member

This particular issue seems to happen because notification list height is fixed at 70vh. Since the callback for checking whether we're at the bottom of the page and loading more happens on scroll, if the entire first page of content fits in those 70vh, there won't be a scrollbar, so there will never be scrolling, so nothing new will be loaded.

@davwheat
Copy link
Member

@askvortsov1 We could force the inner container to be calc(70vh + 1px) as a workaround 🤔

@askvortsov1
Copy link
Member

@askvortsov1 We could force the inner container to be calc(70vh + 1px) as a workaround

But then there'd always be a scrollbar, even when unnecessary.

@davwheat
Copy link
Member

davwheat commented May 6, 2021

@askvortsov1 We could check the links of the notifications XHR response. If links.next exists, then there are more notifications, otherwise there aren't.

This could be used to force the inner container to be 70vh + 1px if more notifications are available.

@davwheat davwheat self-assigned this May 6, 2021
@dsevillamartin
Copy link
Member

We could check the links of the notifications XHR response. If links.next exists, then there are more notifications, otherwise there aren't.

This can be done once flarum/framework#2781 is merged, as it includes hasNext methods and stuff.

@askvortsov1 askvortsov1 transferred this issue from flarum/framework Mar 10, 2022
@SychO9 SychO9 removed this from Roadmap May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants