Feature/sc 26865/sheets desktop topics display sheets only #2398
Conversation
…navigation' into feature/sc-26865/sheets-desktop-topics-display-sheets-only
… sheets api error
…navigation' into feature/sc-26865/sheets-desktop-topics-display-sheets-only
stevekaplan123
changed the base branch from
modularization-main
to
feature/sc-32283/sheets-desktop-update-topics-homepage-navigation
There was a problem hiding this comment.
Pull Request Overview
This PR modularizes topic page components to support displaying sheet sources exclusively in the 'sheets' module while preserving the library source display in the 'library' module. Key changes include updating processTopicsData to properly filter and rename tabs based on active modules, renaming tab keys to use consistent, capitalized naming, and integrating new components (TopicPageTabs and AuthorIndexItems) into TopicPage for improved readability and maintainability.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| static/js/sefaria/sefaria.js | Updates the processTopicsData function to filter references by module and refines tab key naming. |
| static/js/TopicPage.jsx | Adjusts tab key values, updates conditional checks accordingly, and integrates new components to modularize the topic page UI. |
Comments suppressed due to low confidence (3)
static/js/sefaria/sefaria.js:2916
- Ensure that the capitalization changes for tab keys (e.g., 'Notable Sources') are consistently updated across all related translation and comparison functions.
if (tabs["Notable Sources"]) {
static/js/TopicPage.jsx:661
- Verify that the tab name comparisons here match the new, capitalized naming convention to prevent any logical regressions in tab selection.
if (displayTabs.length && tab !== "Notable Sources" && tab !== "Author Works on Sefaria") {
static/js/TopicPage.jsx:803
- [nitpick] Consider extracting the AuthorIndexItems and TopicPageTabs components into separate modules to improve maintainability and adhere to a clearer separation of concerns.
const AuthorIndexItems = ({topicData}) => {
stevekaplan123
changed the base branch from
feature/sc-32283/sheets-desktop-update-topics-homepage-navigation
to
modularization-main
stevekaplan123
changed the base branch from
modularization-main
to
feature/sc-32283/sheets-desktop-update-topics-homepage-navigation
stevekaplan123
changed the base branch from
feature/sc-32283/sheets-desktop-update-topics-homepage-navigation
to
master
stevekaplan123
changed the base branch from
master
to
feature/sc-32283/sheets-desktop-update-topics-homepage-navigation
There was a problem hiding this comment.
Copilot reviewed 2 out of 3 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 (1)
static/js/TopicPage.jsx:587
- React hooks must be called unconditionally in the top-level of a component or custom hook. Calling useIncrementalLoad within the helper function setupDisplayTabs may violate the rules of hooks; consider refactoring setupDisplayTabs into a custom hook.
useIncrementalLoad(tabObj.fetcher, refsToFetchByTab[key] || false, Sefaria._topicPageSize, data => onIncrementalLoad(data, key), topic);
There was a problem hiding this comment.
Pull Request Overview
This PR implements the modularization of topic pages so that when viewing topics in the sheets module only sheet sources are displayed. Key changes include:
- Refactoring of the TopicPage component and extraction of helper functions to set up tabs.
- Updating the topic API processing by replacing processTopicsData with processTopicsTabsData and introducing a new helper method (_createTabKeyAndTitle).
- Adjustments in the TopicPageTab view and bulk sheet fetch to correctly handle sheet identifiers.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| static/js/sefaria/strings.js | Added new localized string for "Top Citations." |
| static/js/sefaria/sefaria.js | Updated topic API processor to use processTopicsTabsData and added helper method _createTabKeyAndTitle. |
| static/js/TopicPage.jsx | Refactored TopicPage component, renamed hooks and updated sheet fetching logic. |
Files not reviewed (1)
- static/css/s2.css: Language not supported
static/js/sefaria/sefaria.js
Outdated
| for (let [linkType, topicLinks] of Object.entries(topicData.refs)) { | ||
| let refs = topicLinks.refs.filter(refObj => (Sefaria.activeModule === 'sheets') === refObj.is_sheet); | ||
| for (let refObj of refs) { | ||
| const {tabKey, title} = _createTabKeyAndTitle(linkType, refObj); |
There was a problem hiding this comment.
Since _createTabKeyAndTitle is defined as a method on the Sefaria object, ensure that it is invoked with the correct context (e.g. this._createTabKeyAndTitle(linkType, refObj)) to avoid undefined behavior.
| const {tabKey, title} = _createTabKeyAndTitle(linkType, refObj); | |
| const {tabKey, title} = Sefaria._createTabKeyAndTitle(linkType, refObj); |
Copilot uses AI. Check for mistakes.
I don't see how to immediately make TopicPageTabView work based on purely reusable components, although I noticed a few things that could be cleaned up. First of all, every client of TabView creates an array 'tabs' which is just a list of javascript objects and the client also passes a renderTab function to TabView. These renderTab functions are very similar to each other and it seems like we should just have a component Tab that handles the data passed to the array and also can render itself. I also found one glaring issue that entangles TabView (which we use many times on the site) to TopicPageTabView. Strangely, the filter button and lang toggle for TopicPageTabView are considered tabs which makes no sense conceptually and also entangles the two components together more than they need to be. In the CollectionsPage we also consider filter to be a tab so that component could also be cleaned up by separating filter/lang toggles out of TabView. |
…op-topics-display-sheets-only
stevekaplan123
changed the base branch from
feature/sc-32283/sheets-desktop-update-topics-homepage-navigation
to
master
stevekaplan123
changed the base branch from
master
to
feature/sc-32283/sheets-desktop-update-topics-homepage-navigation
…navigation' into feature/sc-26865/sheets-desktop-topics-display-sheets-only
I agree @stevekaplan123, I would love for us to turn the filter button and language toggle into separate components and not tabs. But I don't think it should be done in this PR, we should create a separate story and PR so as not to break the scope here. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements the display logic change for topic pages in the sheets module by refactoring topics data processing and splitting the TopicPage component into smaller components. Key changes include updating the topics data processor in sefaria.js, refactoring TopicPageTab view and related fetch functions in TopicPage.jsx, and adding a new translation string in strings.js.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| static/js/sefaria/strings.js | Added a new translation string for "Top Citations". |
| static/js/sefaria/sefaria.js | Replaced processTopicsData with processTopicsTabsData and added helper _deriveTabDataForTopicLink for proper tab mapping. |
| static/js/TopicPage.jsx | Refactored fetchBulkSheet and TopicPage logic; reorganized tab setup and useEffect cleanups. |
| static/css/s2.css | Updated CSS selectors for clamped text in card descriptions. |
| } else { | ||
| setLoadedData(prev => ({...prev, [tabKey]: tabObj.loadedData})); | ||
| } | ||
| } | ||
| })()); | ||
| promise.catch((error) => { if (!error.isCanceled) { console.log('TopicPage Error', error); } }); |
There was a problem hiding this comment.
The promise returned from the self-invoking async function is not stored in a variable before calling 'promise.catch'. Assign the result of the async invocation to a variable (e.g., 'const promise = (async () => { ... })();') to handle errors properly.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
static/js/sefaria/sefaria.js
Outdated
| } | ||
| else { | ||
| if (linkType === 'about') { | ||
| const lang = Sefaria.interfaceLang === "hebrew" ? 'he' : 'en'; |
There was a problem hiding this comment.
i think Sefaria.interfaceLang.slice(0, 2) is more readable.
There was a problem hiding this comment.
@YishaiGlasner I don't think that's so readable either 🙈
@stevekaplan123 maybe better to add a helper function to Sefaria.js that returns he or en? I.e that encapsulates this logic? It feels like we have it in lots of places...
There was a problem hiding this comment.
@YishaiGlasner @yitzhakc I'm creating a helper function Sefaria._getShortInterfaceLang()
How does this sound?
It can replace 15-20 cases throughout the site
static/js/sefaria/sefaria.js
Outdated
| } | ||
| else { | ||
| if (linkType === 'about') { | ||
| const lang = Sefaria.interfaceLang === "hebrew" ? 'he' : 'en'; |
There was a problem hiding this comment.
@YishaiGlasner I don't think that's so readable either 🙈
@stevekaplan123 maybe better to add a helper function to Sefaria.js that returns he or en? I.e that encapsulates this logic? It feels like we have it in lots of places...
stevekaplan123
requested review from
yitzhakc and
YishaiGlasner
and removed request for
yonadavGit
stevekaplan123
merged commit 8f64e2a
into
feature/sc-32283/sheets-desktop-update-topics-homepage-navigation
Description
This PR implements a requirement for modularization -- while in the 'sheets' module, topic pages should only display sheet sources rather than library sources.
Code Changes