Feature/sc 32283/sheets desktop update topics homepage navigation #2338

stevekaplan123 · 2025-02-18T14:12:48Z

Description

This PR implements a topic TOC for the sheets module. 1) As opposed to the library module, the topics TOC is not the topics landing page but is the old TopicsPage component. 2) The Topics TOC in the sheets module only shows topics that have sheets associated with them. 3) When viewing a topic category in either the library or sheets module, we only show topics that are appropriate for the given module. In other words, in the sheets module, subtopics are only shown if that subtopic has sheets associated with it, and likewise, in the library module, subtopics are only shown if they have library sources associated with them.

Code Changes

I created a new component TopicTOCCard to display topics throughout the topic TOC. The TopicsPage component and TopicCategory components now use TopicTOCCard.
Most of the changes in this PR were from this PR. There is very little in this PR specifically required to implement the sheets module topics TOC. The logic is basically just that (a) when building the topic TOC on the backend in model/text.py in get_topic_toc_json_recursive, we add a new property to each topic object called pools, which is just an array of the topic pools that the topic belongs to. Then, when we already on a topic category page, such as prayer, when we render the TopicCategory component, we call a function shouldDisplay to determine if each subtopic (such as Aleinu) actually belongs to the sheets/library topic pool. See the function shouldDisplay in TopicPage.jsx where we filter out all subtopics that are not appropriate for the given Sefaria.activeModule:

const TopicCategory = ({topic, topicTitle, setTopic, setNavTopic, initialWidth,
  openSearch}) => {
    const [topicData, setTopicData] = useState(Sefaria.getTopicFromCache(topic) || {primaryTitle: topicTitle});
    const [subtopics, setSubtopics] = useState(Sefaria.topicTocPage(topic));
@@ -226,38 +230,15 @@ const TopicCategory = ({topic, topicTitle, setTopic, setNavTopic, compare, initi
        setSubtopics(Sefaria.topicTocPage(topic));
    }, [topic]);

    const shouldDisplay = (t) => {
      const inActiveModule = t.pools.includes(Sefaria.activeModule);
      return t.shouldDisplay !== false && inActiveModule;
    }

    let topicBlocks = subtopics
      .filter(shouldDisplay)
      .sort(Sefaria.sortTopicsCompareFn)
      .map((topic, i) => <TopicTOCCard topic={topic} setTopic={setTopic} setNavTopic={setNavTopic} key={i}/>);

…o-sheets-pool-when-user-tags-topic

…ature/sc-32391/add-topic-to-sheets-pool-when-user-tags-topic

…op-update-topics-homepage-navigation

Copilot

Pull Request Overview

This PR implements module-specific topic displays by supplementing topic entries with a "pools" property and updating the navigation components. Key changes include:

Adding a "pools" field for topics in the TOC in text.py.
Updating TopicPage.jsx to filter topics based on the active module.
Adjusting sidebar components and navigation in NavSidebar.jsx, ReaderPanel.jsx, and TopicsPage.jsx.

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
static/js/TopicPage.jsx	Introduces a new topic filter function and refactors topic block rendering.
static/js/NavSidebar.jsx	Adds a new sidebar module component "JoinTheConversation".
sefaria/urls.py	Introduces new URL routes for sheets topics.
static/js/ReaderPanel.jsx	Updates menu rendering based on the active module.
static/js/TopicsPage.jsx	Modifies sidebar module list to include "JoinTheConversation".
sefaria/model/text.py	Adds support for a "pools" key in the topic TOC JSON output.

Files not reviewed (1)

static/css/s2.css: Language not supported

static/js/TopicPage.jsx

…navigation' into feature/sc-26865/sheets-desktop-topics-display-sheets-only

YishaiGlasner · 2025-03-20T08:28:33Z

sefaria/model/text.py

@@ -5157,7 +5158,8 @@ def get_topic_toc_json_recursive(self, topic=None, explored=None, with_descripti
                "shouldDisplay": True if len(children) > 0 else topic.should_display(),
                "en": topic.get_primary_title("en"),
                "he": topic.get_primary_title("he"),
-                "displayOrder": getattr(topic, "displayOrder", 10000)
+                "displayOrder": getattr(topic, "displayOrder", 10000),
+                "pools": DjangoTopic.objects.slug_to_pools[topic.slug]


if i understand correctly, that changes an API response object.

it seems ok for me, but i think those changes (although small) should be considered from architecture POV.

IMHO it would be better to add reader_views.topic_page a param that says if it's for library or sheet, and mpving the filtering logics to the server side.

if we have this response in the dev portal, it should be reflected there also, so you should change the json accordingly.

@YishaiGlasner Are you sure this changes an API response? Which API url does it change?

Re 2. I do agree that we should take advantage of the request.active_module attribute set in the ModuleMiddleware to either sheets or library to take advantage of filtering the list of topics on the backend, although I see no problem having the pools remain on this object, since it's cached anyways.

@yitzhakc i think it adds the key pools to topics/

@YishaiGlasner @yitzhakc I don't think this changes any API results. This changes the Sefaria._topics cache which is handled by the front-end only. /api/topics is not affected by this. And yes, this adds a 'pools' key to every topic in the Sefaria._topics cache. The reason I did it this way is because reader_views.topic_page only gets called if someone loads a topic page server side. Therefore, if you navigate to "Explore" and then go to a topic page, reader_views.topic_page won't get called. Instead, the front-end will use the Sefaria._topics cache. The problem is that this cache gets set on the backend by get_topic_toc_json (which is in turn called by large_data in context_processors.py) when the server initially loads. This happens long before any request is received with an active_module parameter. Therefore, I don't see how we could take advantage of the active_module parameter in the request.

static/js/NavSidebar.jsx

static/js/ReaderPanel.jsx

static/js/TopicPage.jsx

… sheets api error

yitzhakc · 2025-03-31T08:38:45Z

sefaria/model/text.py

@@ -5157,7 +5158,8 @@ def get_topic_toc_json_recursive(self, topic=None, explored=None, with_descripti
                "shouldDisplay": True if len(children) > 0 else topic.should_display(),
                "en": topic.get_primary_title("en"),
                "he": topic.get_primary_title("he"),
-                "displayOrder": getattr(topic, "displayOrder", 10000)
+                "displayOrder": getattr(topic, "displayOrder", 10000),
+                "pools": DjangoTopic.objects.slug_to_pools[topic.slug]


@YishaiGlasner Are you sure this changes an API response? Which API url does it change?

Re 2. I do agree that we should take advantage of the request.active_module attribute set in the ModuleMiddleware to either sheets or library to take advantage of filtering the list of topics on the backend, although I see no problem having the pools remain on this object, since it's cached anyways.

sefaria/urls.py

static/js/ReaderPanel.jsx

static/js/TopicPage.jsx

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…-topics-display-sheets-only Feature/sc 26865/sheets desktop topics display sheets only

…op-update-topics-homepage-navigation

Copilot

Pull Request Overview

This PR adds a topic table of contents (TOC) card component and refactors both client and server logic to support module-specific topic navigation (library vs. sheets). Key changes include

New TopicTOCCard component and refactoring of TopicsPage/TopicCategory to use it
Backend updates (processTopicsTabsData, URL patterns, view logic) to handle sheets vs. library pools and module-specific topic tabs
Introduction of _getShortInterfaceLang helper and widespread refactoring of interface-language slicing

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
static/js/common/TopicTOCCard.jsx	Added reusable TOC card for topics
static/js/sefaria/sefaria.js	Replaced `processTopicsData` with `processTopicsTabsData`; added tab derivation, module-specific trending, and `_getShortInterfaceLang`
static/js/TopicsPage.jsx	Switched to `TopicTOCCard` for root topics page; updated sidebar
sourcesheets/views.py	Updated trending tags API to accept `n` parameter
sefaria/urls.py	Added `/sheets/topics/` URL patterns
sefaria/model/text.py	Included `pools` in topic TOC JSON
reader/views.py	Refactored `topic_page` view for slug parameter and module checks
[Various other JS/CSS files]	Refactored language-slice logic; added join-conversation sidebar module and styles

sourcesheets/views.py

reader/views.py

sourcesheets/views.py

static/js/TopicPage.jsx

static/js/common/TopicTOCCard.jsx

…op-update-topics-homepage-navigation

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

This PR implements a module-aware topic table of contents (TOC) for the sheets module by extending topic card rendering and backend tab logic.

Introduces TopicTOCCard for consistent topic listing across pages
Refactors topic data processing to assign refs into module-specific tabs
Adds pools metadata on topics and filters subtopics by active module

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
static/js/sefaria/sefaria.js	Adds `processTopicsTabsData` and `_deriveTabDataForTopicLink`, updates topic caching to use module-specific pools
static/js/common/TopicTOCCard.jsx	New component for rendering topic cards in category pages
static/js/TopicsPage.jsx	Replaces manual category listing with `TopicTOCCard` and updates sidebar modules
static/js/TopicPageAll.jsx	Switches to central `_getShortInterfaceLang` helper
static/js/TopicLandingPage/TopicsLandingPage.jsx	Uses generic `TrendingTopics` in sidebar
static/js/TopicLandingPage/TopicLandingNewsletter.jsx	Uses `_getShortInterfaceLang` for newsletter analytics
static/js/TopicEditor.jsx	Simplifies language selection via `Sefaria._v`
static/js/Story.jsx	Centralizes short-code language helper
static/js/StaticPages.jsx	Uses `_getShortInterfaceLang` for locale in sorting
static/js/SourceEditor.jsx	Applies short-code helper for sheet descriptions
static/js/ReaderPanel.jsx	Renders `TopicsPage` in sheets module and `TopicsLandingPage` in library
static/js/ReaderApp.jsx	Adopts `_getShortInterfaceLang` for interface language slicing
static/js/NavSidebar.jsx	Adds "JoinTheConversation" module and cleans up trending mapping
static/css/s2.css	Styles `.joinTheConversation` and clamps card descriptions
sourcesheets/views.py	Validates `n` parameter in trending tags API
sefaria/utils/util.py	Adds `get_short_lang` helper
sefaria/urls.py	Adds routes for sheets topic pages
sefaria/model/text.py	Extends TOC JSON with `pools` for module filtering
reader/views.py	Applies `get_short_lang` in view logic and enforces module access

Comments suppressed due to low confidence (4)

static/js/sefaria/sefaria.js:2858

[nitpick] The new processTopicsTabsData function introduces module-specific tab assignment logic but lacks accompanying unit tests. Consider adding tests to verify correct tab grouping for both library and sheets modules.

processTopicsTabsData: function(topicData) {

static/js/common/TopicTOCCard.jsx:10

The openTopic handler references children, slug, en, and he before they are declared (TDZ error). Move the destructuring of topic above this handler or reference topic.slug etc. directly inside the function.

children ? setNavTopic(slug, {en, he}) : setTopic(slug, {en, he});

static/js/ReaderPanel.jsx:951

TopicsPage is used here but not imported in this file, causing a ReferenceError. Add import { TopicsPage } from './TopicsPage'; at the top.

menu = <TopicsPage

sourcesheets/views.py:966

This error response returns JSON without an HTTP error status. Consider returning a 400 Bad Request (e.g., jsonResponse(..., status=400)) to more clearly signal client input error.

return jsonResponse({"error": "Invalid value for parameter 'n'. It must be an integer."})

Copilot

Pull Request Overview

This PR implements an updated topics table-of-contents (TOC) for the sheets module that filters topics based on the module (sheets vs. library) and updates related views, components, and APIs.

Introduces a new TopicTOCCard component to standardize topic presentation.
Updates the topics processor to processTopicsTabsData and applies topic pool filtering in the backend.
Adjusts trending topics API calls, URL routes, and various UI components for proper handling of the active module.

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
static/js/sefaria/sefaria.js	Updates topics processor, trending topics functions, and language helpers.
static/js/common/TopicTOCCard.jsx	New component for rendering topic cards with conditional navigation.
static/js/TopicsPage.jsx	Replaces legacy category rendering with TopicTOCCard usage.
static/js/TopicPageAll.jsx	Updates topic sorting to leverage the new language helper.
static/js/TopicLandingPage/*	Adjusts trending topics module and newsletter language handling.
static/js/TopicEditor.jsx, Story.jsx, etc.	Refactors usage of language conversion to Sefaria._getShortInterfaceLang.
static/css/s2.css	Adds styling for card clamping and spacing adjustments.
sourcesheets/views.py	Enhances trending topics API endpoint with parameter validation.
sefaria/utils/util.py	Adds a helper function to convert full language strings to short codes.
sefaria/urls.py	Introduces new URL routes for sheets topics.
sefaria/model/text.py	Adds new property ‘pools’ for topics to facilitate module filtering.
reader/views.py	Updates topic page processing to normalize slugs and filter topics by active module.

Comments suppressed due to low confidence (1)

static/js/common/TopicTOCCard.jsx:8

The openTopic function uses the 'children' variable before its destructuring assignment. Consider moving the destructuring of { slug, children } to before the definition of openTopic to ensure the variable is defined when used.

const openTopic = e => { e.preventDefault(); children ? setNavTopic(slug, {en, he}) : setTopic(slug, {en, he}); }

sefaria/utils/util.py

static/js/TopicPage.jsx

stevekaplan123 added 6 commits February 18, 2025 11:11

Merge branch 'modulrization-themes' into feature/sc-32391/add-topic-t…

ffdbbfd

…o-sheets-pool-when-user-tags-topic

chore: sheets/topics works

f64fc77

chore: add JoinTheConversation

c82053c

Merge branch 'modukarization-themes-sheets-homepage-refactor' into fe…

8c25d93

…ature/sc-32391/add-topic-to-sheets-pool-when-user-tags-topic

chore: css fix for joinTheConversation

e4f39b7

Merge branch 'modularization-main' into feature/sc-32283/sheets-deskt…

e434929

…op-update-topics-homepage-navigation

stevekaplan123 changed the base branch from modularization-main to modukarization-themes-sheets-homepage-refactor March 12, 2025 10:14

stevekaplan123 changed the base branch from modukarization-themes-sheets-homepage-refactor to modulrization-themes March 12, 2025 10:15

stevekaplan123 changed the base branch from modulrization-themes to modularization-main March 12, 2025 10:15

stevekaplan123 added 3 commits March 12, 2025 14:15

chore: add pools attribute to topic_toc entries

c3a3927

chore: bring back sheet possibility in processTopicsData

a6c62be

Merge branch 'modularization-main' into feature/sc-32283/sheets-deskt…

6c272e6

…op-update-topics-homepage-navigation

stevekaplan123 requested review from Copilot, yitzhakc and YishaiGlasner March 20, 2025 08:11

stevekaplan123 marked this pull request as ready for review March 20, 2025 08:11

Copilot AI reviewed Mar 20, 2025

View reviewed changes

static/js/TopicPage.jsx Outdated Show resolved Hide resolved

Merge branch 'feature/sc-32283/sheets-desktop-update-topics-homepage-…

b9cb496

…navigation' into feature/sc-26865/sheets-desktop-topics-display-sheets-only

YishaiGlasner reviewed Mar 20, 2025

View reviewed changes

stevekaplan123 added 3 commits March 20, 2025 14:39

chore: refactor topicpage code and display sheet and sources tabs but…

7c97f3c

… sheets api error

chore: fix sheet parse

8fc941f

chore: remove lang toggle

eb482dc

yitzhakc requested changes Mar 31, 2025

View reviewed changes

stevekaplan123 added 4 commits March 31, 2025 13:23

chore: make topicnavblock its own component

7efb164

chore: differentiate topics_page based on sheets vs library

bc5334b

chore: change topic to slug

80f9af2

chore: make TopicCard

33bbc20

stevekaplan123 requested review from YishaiGlasner, yitzhakc and Copilot April 1, 2025 14:00

stevekaplan123 and others added 5 commits May 12, 2025 00:24

Update static/js/sefaria/sefaria.js

44fff4b

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

chore: _getShortInterfaceLang

e4e0f66

chore: add more comments and clean up code

9b6cf05

Merge pull request #2398 from Sefaria/feature/sc-26865/sheets-desktop…

8f64e2a

…-topics-display-sheets-only Feature/sc 26865/sheets desktop topics display sheets only

Merge branch 'modularization-main' into feature/sc-32283/sheets-deskt…

06438d6

…op-update-topics-homepage-navigation

stevekaplan123 changed the base branch from modularization-main to feature/sc-26865/sheets-desktop-topics-display-sheets-only May 25, 2025 08:59

stevekaplan123 changed the base branch from feature/sc-26865/sheets-desktop-topics-display-sheets-only to master May 25, 2025 09:01

stevekaplan123 changed the base branch from master to feature/sc-26865/sheets-desktop-topics-display-sheets-only May 25, 2025 09:04

stevekaplan123 requested review from yitzhakc, Copilot and YishaiGlasner May 25, 2025 09:16

stevekaplan123 marked this pull request as ready for review May 25, 2025 09:18

Copilot AI reviewed May 25, 2025

View reviewed changes

sourcesheets/views.py Outdated Show resolved Hide resolved

stevekaplan123 changed the base branch from feature/sc-26865/sheets-desktop-topics-display-sheets-only to modularization-main May 25, 2025 09:19

yitzhakc requested changes May 27, 2025

View reviewed changes

reader/views.py Outdated Show resolved Hide resolved

sourcesheets/views.py Outdated Show resolved Hide resolved

static/js/TopicPage.jsx Show resolved Hide resolved

static/js/TopicPage.jsx Show resolved Hide resolved

static/js/common/TopicTOCCard.jsx Outdated Show resolved Hide resolved

stevekaplan123 and others added 4 commits May 28, 2025 09:55

chore: get_short_lang

5cc15ef

Merge branch 'modularization-main' into feature/sc-32283/sheets-deskt…

2743e5b

…op-update-topics-homepage-navigation

chore: add comments

0134163

Update sourcesheets/views.py

e70f746

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

stevekaplan123 force-pushed the feature/sc-32283/sheets-desktop-update-topics-homepage-navigation branch from e69039b to e70f746 Compare May 29, 2025 08:57

stevekaplan123 requested review from yitzhakc and Copilot May 29, 2025 09:01

Copilot AI reviewed May 29, 2025

View reviewed changes

stevekaplan123 requested a review from Copilot May 29, 2025 09:08

Copilot AI reviewed May 29, 2025

View reviewed changes

yitzhakc requested changes May 29, 2025

View reviewed changes

sefaria/utils/util.py Outdated Show resolved Hide resolved

static/js/TopicPage.jsx Show resolved Hide resolved

chore: validate input

8602ffc

stevekaplan123 requested a review from yitzhakc June 3, 2025 10:09

yitzhakc approved these changes Jun 4, 2025

View reviewed changes

stevekaplan123 merged commit 8f6c13b into modularization-main Jun 4, 2025
21 of 27 checks passed

Uh oh!

Feature/sc 32283/sheets desktop update topics homepage navigation #2338

Feature/sc 32283/sheets desktop update topics homepage navigation #2338

Uh oh!

Conversation

stevekaplan123 commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Code Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

YishaiGlasner Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

yitzhakc Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

YishaiGlasner Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

stevekaplan123 Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yitzhakc Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

stevekaplan123 commented Feb 18, 2025 •

edited

Loading