-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Add fix remove iframe #181
base: master
Are you sure you want to change the base?
Conversation
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
Because according to my conclusion it is unnecessary. |
What browsers were tested? Additionally, removing the |
Ipad Mini, Safari. Look at the issue here
#182
…On Mon, Jun 24, 2019 at 4:16 PM Jason Day ***@***.***> wrote:
What browsers were tested?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#181?email_source=notifications&email_token=AE3K2K7FSMYXO2PES55YMKDP4FBTTA5CNFSM4H3BAR2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYOMNRI#issuecomment-505202373>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE3K2K4GKBJ2IUJBH2U7UPLP4FBTTANCNFSM4H3BAR2A>
.
|
I can explain you more details if we could have a small call.
On Mon, Jun 24, 2019 at 4:24 PM Roberto Salazar <robertopccon@gmail.com>
wrote:
… Ipad Mini, Safari. Look at the issue here
#182
On Mon, Jun 24, 2019 at 4:16 PM Jason Day ***@***.***>
wrote:
> What browsers were tested?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#181?email_source=notifications&email_token=AE3K2K7FSMYXO2PES55YMKDP4FBTTA5CNFSM4H3BAR2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYOMNRI#issuecomment-505202373>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AE3K2K4GKBJ2IUJBH2U7UPLP4FBTTANCNFSM4H3BAR2A>
> .
>
|
@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. |
@jasonday PR is updated, please take a look. |
Thanks, I'll look further. I worry that the "framing" of the new option may be confusing, as |
@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. |
// after print callback | ||
if (typeof opt.afterPrint === "function") { | ||
if (opt.keepIframe) { | ||
$iframe.remove(); |
There was a problem hiding this comment.
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.
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.