-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Code maintenance/59280 ensure that the new activity tab renders quickly also for work packages with over 100 comments #17224
Code maintenance/59280 ensure that the new activity tab renders quickly also for work packages with over 100 comments #17224
Conversation
…rk packages with over 100 comments https://community.openproject.org/work_packages/59280
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue with scrolling on mobile - iOS Safari
https://community.openproject.org/api/v3/file_links/2093/open
…further rendering optimizations
…he-new-activity-tab-renders-quickly-also-for-work-packages-with-over-100-comments
… will be fully cleaned up in other PR
…he-new-activity-tab-renders-quickly-also-for-work-packages-with-over-100-comments
Co-authored-by: Kabiru Mwenja <k.mwenja@openproject.com>
…be done in the feature flag removal PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Jonas! I push some test coverage for the notifications eager loading 👍🏾
journals.each do |journal| | ||
journal.instance_variable_set( | ||
:@unread_notifications, | ||
notifications[journal.id] | ||
) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very smart 👍🏾
@@ -375,6 +375,7 @@ def rerender_journals_with_updated_notification(journals, last_update_timestamp, | |||
# alternative approach in order to bypass the notification join issue in relation with the sequence_version query | |||
Notification | |||
.where(journal_id: journals.pluck(:id)) | |||
.where(recipient_id: User.current.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏾
Ticket