Skip to content

Add fix remove iframe#181

Open
rpccon wants to merge 2 commits into
jasonday:masterfrom
rpccon:feature/IFRAME-REMOVE
Open

Add fix remove iframe#181
rpccon wants to merge 2 commits into
jasonday:masterfrom
rpccon:feature/IFRAME-REMOVE

Conversation

@rpccon

@rpccon rpccon commented Jun 24, 2019

Copy link
Copy Markdown

Remove iframe should be until the print view be shown. Because if you are using the Ipad Mini, after the first time he ask the user if allow or deny the request and according to your library just give a second to show the print view, I mean when Ipad show that pop-up, lost that second, and when user press allow, just appear an empty page, that's because the Iframe has been removed.

@rpccon

rpccon commented Jun 24, 2019

Copy link
Copy Markdown
Author

The main change is in the line 317. I added the remove iframe line inside there, because I think there is the right moment to do it.

Also I have removed

                if (!opt.debug) {
                    setTimeout(function() {
                        

                    }, 1000);
                }

Because according to my conclusion it is unnecessary.

@jasonday

jasonday commented Jun 24, 2019

Copy link
Copy Markdown
Owner

What browsers were tested?

Additionally, removing the if(!opt.debug) logic will break the debug functionality. That logic would need to be in place within your change to line 317. Please make that change, so that testing can proceed.

@rpccon

rpccon commented Jun 24, 2019 via email

Copy link
Copy Markdown
Author

@rpccon

rpccon commented Jun 24, 2019 via email

Copy link
Copy Markdown
Author

@rpccon

rpccon commented Jun 24, 2019

Copy link
Copy Markdown
Author

@jasonday I understand you point, maybe if we create another prop in defaults object, to keep the debug functionality and my point working is a good idea for me, I'll update the PR.

@rpccon

rpccon commented Jun 24, 2019

Copy link
Copy Markdown
Author

@jasonday PR is updated, please take a look.

@jasonday

Copy link
Copy Markdown
Owner

Thanks, I'll look further. I worry that the "framing" of the new option may be confusing, as keepIframe indicates that the iframe is kept, not removed. I think there's probably a way to include your original idea without the additional option.

@rpccon

rpccon commented Jun 25, 2019

Copy link
Copy Markdown
Author

@jasonday We just need a way to stop the remove of the iframe when debug is false (set it true is not a way). If you can apply a change to do it I'll be grateful. I think in aferprint function is a good location to do it.

Comment thread printThis.js
// after print callback
if (typeof opt.afterPrint === "function") {
if (opt.keepIframe) {
$iframe.remove();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This would fire even sooner, because afterprint does not wait; it depends on the browser interrupting execution.

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.

3 participants