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

fix: infinite scroll not initialized for notifications on big screens #3733

Merged
merged 2 commits into from
Apr 17, 2023

Conversation

rob006
Copy link
Contributor

@rob006 rob006 commented Feb 11, 2023

Changes proposed in this pull request:

On QHD screens 70vh is bigger than space taken by 10 notifications. As a result there is no scroll for this dropdown, and as a result infinite scroll do not work in this place. I mentioned this on Discord a few months ago: https://discord.com/channels/360670804914208769/707030467450372166/1010096800511836261

This PR sets max height of this dropdown to 800px and increases number of notifications loaded in this dropdown, to make sure it is always filled.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@rob006 rob006 requested a review from a team as a code owner February 11, 2023 22:40
@iamdarkle
Copy link
Member

Coincidentally, I experienced this for the first time last week, and it was on a 1080p screen, in a particular case where the whole list of notifications were from the same discussion.

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

The max-height change makes sense, but I'm hesitant to remove pagination-by-default from the notifications list controller like that. Couldn't that be a frontend change, with a different limit query param depending on screen size?

@rob006
Copy link
Contributor Author

rob006 commented Apr 16, 2023

but I'm hesitant to remove pagination-by-default from the notifications list controller like that

This PR does not remove pagination, it changes page size from 10 to 20 items (this is default size from AbstractSerializeController).

@SychO9
Copy link
Member

SychO9 commented Apr 16, 2023

rather than 20, 12 should be enough for this case? notifications listing performance is still not great, so ideally we should avoid doubling items per page.

@rob006
Copy link
Contributor Author

rob006 commented Apr 16, 2023

12 will not be enough in some cases. I have many notifications like this:

c7afa43a

In this scenario you need ate least 14-15 notifications to fill 800px of dropdown. And probably there could be cases when you have 4 notifications grouped like this - then you need even more notifications. 20 is just safe value, and performance is not terrible in my case (it is faster than discussions list or single discussion view in most cases).

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

I think this all seems reasonable. I don't see why notifications should have a special case for the default limit.

Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

In this scenario you need ate least 14-15 notifications to fill 800px of dropdown.

Alright then

@SychO9 SychO9 added this to the 1.8 milestone Apr 17, 2023
@SychO9 SychO9 merged commit 1b5da13 into flarum:main Apr 17, 2023
@SychO9 SychO9 changed the title Fix bug when infinite scroll was not initialised for notifications dropdown on big screens fix: infinite scroll not initialized for notifications on big screens Apr 17, 2023
@rob006 rob006 deleted the notifications-dropdown branch April 17, 2023 08:26
@github-actions github-actions bot mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants