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

Allow Skinned Viewers to Change Find Bar without Changing Javascript #5803

Closed

Conversation

speedplane
Copy link

The Problem

For those of use who write our own viewers, we often change or remove certain functionality from the stock viewer. In those situations, it's nice for the javascript to not be so tightly tied to the DOM layout. That way, we can remove/change objects without crashing the viewer or modifying any javascript.

The Solution

In this PR, we remove all javascript dependencies in the find bar's DOM. That is, anyone who skins pdf.js and removes or changes the DOM layout of the find bar will now no longer have to change their javascript too. We do this simply by adding null checks to make sure an element exists before acting on it.

Going Forward

There are many other places in pdf.js where the javascript is tightly tied to the DOM, not just the find bar. If this PR is acceptable, I can try to do the same for other areas as well. I thought I'd start small here though.

…llows those who skin the viewer and possibly remove/change functionality to still be able to use this file unchanged.
@speedplane speedplane changed the title Allow Skinned Viewers to Change Find Bar without Changing DOM Allow Skinned Viewers to Change Find Bar without Changing Javascript Mar 6, 2015
@timvandermeij
Copy link
Contributor

I agree that we need to solve this problem. The only think that should be changed in my opinion is addListenerSafely's position. I think we should at least put it somewhere where all files can use it (since, as you pointed out, this might be used in more places). Maybe web/ui_utils.js? Let's ask @yurydelendik for his opinion on this.

@timvandermeij
Copy link
Contributor

We have had some more discussion about this and we have come to the conclusion that the current PR is not a good solution to the problem. The main reason is that it adds unneeded complexity to the regular viewer in a lot of places. Instead we should be striving for a solution where we decouple the JS code ('model') from the views as much as possible, and we are already working on that with a new viewer API which is much more component-based.

Skinned viewers should be adjusting the viewer to their needs by creating their own components instead of monkey-patching our components. Suggestions to improve the decoupling of model and view without much added complexity are always welcome of course!

I'm therefore closing this PR, but we will track the request in a separate issue so we can take the needs of implementers in mind for the new viewer API.

@timvandermeij
Copy link
Contributor

Follow-up: the problem you described in this PR is now tracked at #5830 so we can share more ideas on how to make this easier for everyone and to still strive for a minimal and reusable solution.

@speedplane
Copy link
Author

Agree, that definitely seems like the better long term approach.

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

Successfully merging this pull request may close these issues.

2 participants