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

Remove unused util $.fn.focusNextInputField #9180

Merged
merged 4 commits into from
May 20, 2024
Merged

Conversation

RayBB
Copy link
Collaborator

@RayBB RayBB commented Apr 29, 2024

Closes #9179

Technical

Testing

Did a fulltext search of the code and these aren't used anywhere I can see but a second check would be good.
Removes one unused piece of code and adds a comment about why we want to keep the rest.

Screenshot

Stakeholders

@RayBB RayBB force-pushed the fix/remove-unused-utils branch from dd78577 to c35172e Compare May 6, 2024 22:35
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Thanks @RayBB! The changes to the excerpts template are causing errors on the book edit page. Switching from string interpolation to string concatenation should fix things.

@@ -66,7 +66,7 @@
{{excerpt}}
<br />
<i>
{{cond(pages, "From page " + pages + ".", "")}}
{{pages ? `From page ${pages}.` : "";}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is causing an error on the edit page. I think that the template parser is having issues with the $ character. Escaping the character (\$) led to some new errors in my browser's console output.

It's probably best to switch back to old-fashioned string concatenation for this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jimchamp good catch... it is fixed as you suggested!

@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label May 9, 2024
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label May 10, 2024
@RayBB RayBB requested a review from jimchamp May 10, 2024 11:04
@cdrini
Copy link
Collaborator

cdrini commented May 13, 2024

Howdy folks, I've been thinking about this one and I think it might be best for us to close this. Even if we remove the places where our jsdef code uses e.g. cond, it's expected that jsdef codeblocks should have access to these python globals. Removing the JS implementations here will make our code brittle, especially since we don't really have strong tests to verify that our python/jsdef templates are valid.

@cdrini
Copy link
Collaborator

cdrini commented May 13, 2024

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

$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.

@RayBB
Copy link
Collaborator Author

RayBB commented May 13, 2024

@cdrini thanks for replying.

Should we also keep focusNextInputField ?

Do we need to keep the window.cond = cond; for jsdef to work?
Or would it be feasible to move these to a window.jsdef.cond so they're not taking up so much at the top of index.js?

@cdrini
Copy link
Collaborator

cdrini commented May 13, 2024

focusNextInputField looks entirely unused; that looks like we can drop 👍 We do need the window.cond exposure in order for jsdef to work atm; if we wanted to change that, we would need to modify the way that the jsdef converts the templates to JS to check a certain global eg a JSDEF global. That would be nice in some future! But a little complicated to achieve. We'd need to change the parser to basically alter all globals to pull them from somewhere else. Here: https://github.com/internetarchive/openlibrary/blob/b46ff5135473c003f191d804bdd3b07cfa2c37cb/openlibrary/plugins/upstream/jsdef.py

@RayBB
Copy link
Collaborator Author

RayBB commented May 13, 2024

@cdrini sounds good to me. I've pushed code with a comment about this and removing focusNextInputField

@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
@mekarpeles mekarpeles assigned cdrini and unassigned jimchamp May 13, 2024
@cdrini cdrini changed the title remove unused utils Remove unused util $.fn.focusNextInputField May 20, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Lgtm! Tested on testing (the only place I can think where this might apply is the "add one?" link for description, which focusses on the description box; still works!) + can't find any references to that method.

@cdrini cdrini merged commit b2f5505 into master May 20, 2024
4 checks passed
@cdrini cdrini deleted the fix/remove-unused-utils branch May 20, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove unused utils
3 participants