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

Rtl #1859

Draft
wants to merge 73 commits into
base: master
Choose a base branch
from
Draft

Rtl #1859

wants to merge 73 commits into from

Conversation

YishaiGlasner
Copy link
Contributor

comment: this commit remove the APIResult attributes from currVersions object. the currVersions are indeed theoretical currVersions, but the real shown version can be different due to language and version preferences, so the APIResult reflected the real shown versions. i've removed it, for it has no use anymore, but there is a logic in javing this data, so Akiva had suggested to flag it on the review.

…g them to fit the expected old api response.
… to include an object for any language (he and en) rather than a string. The idea is to enable it to include other details and not only versionTitle.
…mbers and use it rather than placeSegmentNuncers.
…o components (rather than get it from cache objects).
YishaiGlasner and others added 27 commits April 21, 2024 09:06
…nt language if it's hebrew or english and in the interdace language otherwise.
@@ -280,6 +281,18 @@ def user_credentials(request):
return {"user_type": "API", "user_id": apikey["uid"]}


def _reader_redirect_add_languages(request, tref):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ssr

@@ -659,12 +659,13 @@ def modify_by_function(self, func, start_sections=None, _cur=None, _curSections=
Func should accept two parameters: 1) text of current segment 2) zero-indexed indices of segment
:param start_sections: array(int), optional param. Sections passed to `func` will be offset by `start_sections`, if passed
"""
_curSections = _curSections or []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ssr

@@ -1484,7 +1484,8 @@ div.interfaceLinks-row a {
.readerPanel .content {
direction: ltr; /* Even in Hebrew Interface, we want scroll bars on the right */
}
.readerPanel .he {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rtl

@@ -4708,14 +4712,17 @@ body .ui-autocomplete.dictionary-toc-autocomplete .ui-menu-item a.ui-state-focus
.tagsList .enOnly {
direction: ltr;
}
.readerControlsOuter {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

controls

@@ -5315,7 +5323,7 @@ body .ui-autocomplete.dictionary-toc-autocomplete .ui-menu-item a.ui-state-focus
.bilingual .sheetContent .title .he {
display: none;
}
.interface-hebrew .readerPanel.english .textRange,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rtl

@@ -2144,11 +2193,13 @@ _media: {},
return x;
})
},
sectionString: function(ref) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

data fetching (enable function to be used wihtout cache but with passed data. making it always do so would be better, for util function should not get data for itself)

@@ -2351,7 +2402,7 @@ _media: {},
},
areVersionsEqual(v1, v2) {
// v1, v2 are `currVersions` objects stored like {en: ven, he: vhe}
return v1.en == v2.en && v1.he == v2.he;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currVersions refactor

@@ -3156,15 +3207,15 @@ Sefaria.unpackDataFromProps = function(props) {
let settings = {context: 1, enVersion: panel.enVersion, heVersion: panel.heVersion};
//save versions first, so their new format is also saved on text cache
if(panel.text?.versions?.length){
let versions = Sefaria._saveVersions(panel.text.sectionRef, panel.text.versions);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. data fetching (response is different)
  2. we use saveVersions outside

@@ -48,30 +48,22 @@ class Util {
static encodeVtitle(vtitle) {
return vtitle.replace(/\s/g, '_').replace(/;/g, '%3B');
}
static _getVersionParams(version) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

data fetching

.join("");
} else {
return "";
}
}
static getCurrVersionsWithoutAPIResultFields(currVersions) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currVersions refactor

@@ -330,23 +347,16 @@ def get_connections_mode(filter):
else:
return "TextList", False

def make_panel_dict(oref, versionEn, versionHe, filter, versionFilter, mode, **kwargs):
def make_panel_dict(oref, primaryVersion, translationVersion, filter, versionFilter, mode, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

ssr

"en": versionEn,
"he": versionHe,
},
"currVersions": {"en": translationVersion, "he": primaryVersion},
Copy link
Contributor

Choose a reason for hiding this comment

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

ssr

text_family["next"] = oref.next_section_ref().normal() if oref.next_section_ref() else None
text_family["prev"] = oref.prev_section_ref().normal() if oref.prev_section_ref() else None
panel["text"] = text_family
primary_params = [primaryVersion['languageFamilyName'], primaryVersion['versionTitle']]
Copy link
Contributor

Choose a reason for hiding this comment

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

ssr

@@ -490,24 +511,30 @@ def make_sheet_panel_dict(sheet_id, filter, **kwargs):
return panels


def make_panel_dicts(oref, versionEn, versionHe, filter, versionFilter, multi_panel, **kwargs):
def make_panel_dicts(oref, primaryVersion, translationVersion, filter, versionFilter, multi_panel, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

ssr

@@ -564,7 +583,7 @@ def text_panels(request, ref, version=None, lang=None, sheet=None):
kwargs["sidebarSearchQuery"] = request.GET.get("sbsq", None)
kwargs["selectedNamedEntity"] = request.GET.get("namedEntity", None)
kwargs["selectedNamedEntityText"] = request.GET.get("namedEntityText", None)
panels += make_panel_dicts(oref, versionEn, versionHe, filter, versionFilter, multi_panel, **kwargs)
panels += make_panel_dicts(oref, primaryVersion, translationVersion, filter, versionFilter, multi_panel, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

ssr

@@ -280,6 +281,18 @@ def user_credentials(request):
return {"user_type": "API", "user_id": apikey["uid"]}


def _reader_redirect_add_languages(request, tref):
versions = Ref(tref).version_list()
Copy link
Contributor

@akiva10b akiva10b Jun 3, 2024

Choose a reason for hiding this comment

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

ssr: how expensive is this operation? is it problematic for us?

else:
currVersions[key] = None
if kwargs.get('extended notes', 0):
currVersions = {"en": translationVersion, "he": primaryVersion}
Copy link
Contributor

@akiva10b akiva10b Jun 3, 2024

Choose a reason for hiding this comment

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

ssr: can be define a little higher and then on line 382 we can do
"currVersions": currVersions

primary_params = [primaryVersion['languageFamilyName'], primaryVersion['versionTitle']]
primary_params[0] = primary_params[0] or 'primary'
translation_params = [translationVersion['languageFamilyName'], translationVersion['versionTitle']]
translation_params[0] = translation_params[0] or 'translation'
Copy link
Contributor

@akiva10b akiva10b Jun 4, 2024

Choose a reason for hiding this comment

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

ssr: I believe the following is cleaner and will achieve the goal for the params list

    primary_params = [primary_version.get('languageFamilyName', 'primary'), primary_version.get('versionTitle')]
    translation_params = [translation_version.get('languageFamilyName', 'translation'), translation_version.get('versionTitle')]

text_adapter = TextRequestAdapter(oref.section_ref(), [primary_params, translation_params], return_format='wrap_all_entities')
text = text_adapter.get_versions_for_query()
# text['ref'] = oref.normal()
#now we we should add the he and text attributes
Copy link
Contributor

@akiva10b akiva10b Jun 4, 2024

Choose a reason for hiding this comment

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

ssr: this should be its own function so that the logic is scopes and purpose nicely articulated in the function name

if _cur is None:
_cur = self._store
if isinstance(_cur, str):
_curSections = _curSections or [0]
Copy link
Contributor

@akiva10b akiva10b Jun 4, 2024

Choose a reason for hiding this comment

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

ssr: add docstring to explain this

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