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.

Sign up for GitHub

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

Jump to bottom

remove unused utils #9179

Closed
RayBB opened this issue Apr 29, 2024 · 3 comments · Fixed by #9180
Closed

remove unused utils #9179

RayBB opened this issue Apr 29, 2024 · 3 comments · Fixed by #9180
Labels
Affects: Developers Lead: @cdrini Issues overseen by Drini (Staff: Team Lead & Solr, Library Explorer, i18n) [managed] Lead: @jimchamp Issues overseen by Jim (Front-end Lead, BookNotes) [managed] Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] Priority: 4 An issue, but should be worked on when no other pressing work can be done. [managed] Type: Bug Something isn't working. [managed] Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed]

Comments

@RayBB
Copy link
Collaborator

RayBB commented Apr 29, 2024 •
edited by mekarpeles

Problem

I noticed some unused functions in openlibrary/plugins/openlibrary/js/utils.js we can remove them.

truncate, cond

See: https://github.com/internetarchive/openlibrary/pull/9180/files

Context

  • Browser (Chrome, Safari, Firefox, etc):
  • OS (Windows, Mac, etc):
  • Logged in (Y/N): Y
  • Environment (prod, dev, local): prod

Notes from this Issue's Lead

Proposal & constraints

Related files

Stakeholders
@RayBB RayBB added Type: Bug Something isn't working. [managed] Needs: Triage This issue needs triage. The team needs to decide who should own it, what to do, by when. [managed] Needs: Lead labels Apr 29, 2024
@RayBB RayBB mentioned this issue Apr 29, 2024
Merged
@mekarpeles mekarpeles added Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed] Priority: 4 An issue, but should be worked on when no other pressing work can be done. [managed] Affects: Developers Lead: @jimchamp Issues overseen by Jim (Front-end Lead, BookNotes) [managed] Lead: @cdrini Issues overseen by Drini (Staff: Team Lead & Solr, Library Explorer, i18n) [managed] and removed Needs: Triage This issue needs triage. The team needs to decide who should own it, what to do, by when. [managed] Needs: Lead labels Apr 29, 2024
@mekarpeles
Copy link
Member

@cdrini mentions that these functions may be used for js def by our html templator and likely cannot be removed.

@RayBB
Copy link
Collaborator Author

RayBB commented May 6, 2024

I see that cond was sneakily used in one place and fixed that. A second set of eyes to confirm would be welcome.

@github-actions github-actions bot added the Needs: Response Issues which require feedback from lead label May 7, 2024
@cdrini
Copy link
Collaborator

cdrini commented May 13, 2024

Responded on PR, but including here for future readers as well:

To clarify, here is what I mean by jsdef using these files:

openlibrary/openlibrary/templates/subjects.html

Lines 84 to 91 in 1860c25

$jsdef renderSubjects(subjects):
$if len(subjects) > 0:
<span class="subject">
$for s in subjects:
<a href="$s.key">$s.name</a>$cond(loop.last, '', ',')
</span>
$else:
<span class="title"><em>$_("None found.")</em></span>

The cond call here uses the window.cond definition when this template is converted to/called from the JS.

@cdrini cdrini removed the Needs: Response Issues which require feedback from lead label May 13, 2024
@RayBB RayBB added the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Affects: Developers Lead: @cdrini Issues overseen by Drini (Staff: Team Lead & Solr, Library Explorer, i18n) [managed] Lead: @jimchamp Issues overseen by Jim (Front-end Lead, BookNotes) [managed] Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] Priority: 4 An issue, but should be worked on when no other pressing work can be done. [managed] Type: Bug Something isn't working. [managed] Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed]
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove unused util $.fn.focusNextInputField internetarchive/openlibrary
3 participants
@RayBB @mekarpeles @cdrini