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

Feature/sc 27313/remove resources panel #2064

Open
wants to merge 22 commits into
base: sheets-viewer-final
Choose a base branch
from

Conversation

stevekaplan123
Copy link
Contributor

@stevekaplan123 stevekaplan123 commented Oct 14, 2024

Description

In this PR, I removed the resources panel from the sheets viewer (and editor) as well as moving code that would no longer be used or relevant given that the resources panel is no longer part of the sheets viewer (and editor).

Code Changes

  1. I removed every reference to the mode SheetAndConnections as it is no longer possible.
  2. In ConnectionsPanel.jsx, I removed the function isSheet, showSheetNodeConnectionTools, and the prop masterPanelSheetId since the master panel can no longer be a sheet. Likewise, masterPanelMode === "Sheet" cases have been removed. I removed several components no longer needed: AboutSheet, SheetToolsList, SheetNodeConnectionTools, MySheetsList, PublicSheetsList and AboutSheetButtons. Some of these components' code was copied over and is being re-used in components in other PRs for the Sheets Viewer. One particular component, ShareBox, has the exact same functionality that I need for the new Sheets Viewer, and so all I had to do was modify its prop MasterPanelSheetId to SheetId.
  3. In sefaria.js in linkSummary, we used to have to handle the possible of a Nechama Leibowitz sheet showing up as a connection to itself. This sheet was called the excludedSheet. Since this is no longer possible, I removed the code that handles this excludedSheet case. This code is called from ConnectionsSummary.jsx which was also modified.
  4. Previously, handleSheetSegmentClick in ReaderPanel used to call handleSegmentClick in ReaderApp, which in turn would open a new Resources Panel using openTextListAt. This is no longer necessary, so I re-factored handleSheetSegmentClick and handleSegmentClick. Now, handleSegmentClick just deals with the case where a library source was clicked, and handleSheetSegmentClick just sets the state, updating highlightedNode and highlightedRefs based on the sheet segment click. This allowed me to remove the function setSheetHighlight in ReaderApp and the functions openSheetConnectionsInPanel and closeSheetConnectionsInPanel in ReaderPanel as they are no longer called.
  5. I moved the component SheetContent, and its associated components, from Sheet.jsx to SheetContent.jsx, a new file.
  6. Finally, I also removed the function preloadConnections from Sheet.jsx as it is no longer necessary to call the related API since related data was displayed in the resources panel.

@stevekaplan123 stevekaplan123 changed the base branch from modularization-main to sheets-viewer-final October 15, 2024 07:43
@stevekaplan123 stevekaplan123 requested review from edamboritz and yitzhakc and removed request for akiva10b November 6, 2024 10:11
static/js/ConnectionsPanel.jsx Outdated Show resolved Hide resolved
static/js/ConnectionsPanel.jsx Outdated Show resolved Hide resolved
@@ -0,0 +1,520 @@
import Component from "react-class";
Copy link
Contributor

Choose a reason for hiding this comment

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

When moving the logic here from static/js/Sheet.jsx did you copy-paste or use git cp? That would preserve the file's history, as opposed to showing that you wrote everything with the current commit date :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know about 'git cp'. I'm wondering if I should try reverting the commit that moved the SheetContent component into its own file and then using 'git cp'. I'm worried though that this might cause a mess downstream in the other branches that are dependent on SheetContent.jsx. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants