Feature/sc 31715/sheets desktop notifications page #2410
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ops, frontend work
…hich doesnt speed things up
…d of hard-coded value used in development
There was a problem hiding this comment.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- static/css/s2.css: Language not supported
Comments suppressed due to low confidence (2)
sefaria/model/user_profile.py:600
- Consider reviewing whether unread_notification_count should also support a scope parameter for consistency with recent_notifications. A consistent behavior may reduce potential confusion in production.
# TODO: Why do we not need to scope the notifications to the module here?
sefaria/model/notification.py:356
- [nitpick] The conditional inline operator for setting the query operator may reduce readability. Consider refactoring by assigning the operator to a variable before using it in the query to improve clarity.
query["type"] = {"$in" if scope == 'sheets' else "$nin": Notification.sheets_notification_types}
…lesforce. Interface language and educator fields were switched
…form-sign-up-producing Hebrew newsletter form sign up producing wrong data in Salesforce
Update file and link for 2024 Impact Report
…ndex-title-autocomplete Encode question marks in titles from autocpmleter
fix: don't fail on self.save when django user doesn't have mongo odject
chore(question mark): control for question mark in templating url and…
fix: configure heap size in nodejs deployment
chore(sentry): lower server rate to 1%
Override merging due to failing playwright tests
…e-for-romanian Feature/sc 32313/adding iso code for romanian Overriding merge queue due to failiing playwright tests
chore: Update node memory requirements
This reverts commit ea4e987.
Revert "chore: Update node memory requirements"
Clear cache in node between requests
…op-notifications-page
stevekaplan123
changed the base branch from
modularization-header-main
to
modularization-main
stevekaplan123
previously approved these changes
May 26, 2025
yitzhakc
dismissed
stevekaplan123’s stale review
The merge-base changed after approval.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Configure notifications page to display only sheet-related notifications.
This pull request includes several changes to enhance the notification system by introducing scoped notifications and improving the user interface. The most important changes include adding scope functionality to notifications, updating the user interface to reflect these changes, and modifying the CSS for better styling.
Code Changes
Scope functionality for notifications:
reader/views.py: Addedactive_moduleto the request and used it in various functions to filter notifications by scope. [1] [2] [3] [4]sefaria/model/notification.py: Introducedsheets_notification_typesand added a helper method_build_query_with_scopeto filter notifications by scope. Updated existing methods to use this helper method. [1] [2]sefaria/model/user_profile.py: Modifiedrecent_notificationsandunread_notification_countto accept a scope parameter.sefaria/urls.py: Added a new URL pattern for sheet notifications.User interface updates:
static/js/NotificationsPanel.jsx: Updated the notifications panel to use the scoped notifications and improved the logic for displaying notifications and login prompts. [1] [2]static/js/NotificationsPanel.jsx: Added a new class to style the notification user names. [1] [2] [3]CSS improvements:
static/css/s2.css: Changed the font family for notification titles and summaries to sans-serif and added a new class for styling notification user names. [1] [2]##Notes
I refactored the generation of the notification queries to add awareness of the relevant scope (sheets or library)