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

call widget.resize when slides are shown #199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jjallaire
Copy link
Collaborator

This PR hoists a workaround that I have in dygraphs into the core htmlwidgets. Here is the original code in dygraphs:

https://github.com/rstudio/dygraphs/blob/master/inst/htmlwidgets/dygraphs.js#L131-L144

The issue is that for dygraphs when they start out as display:none (which is the case in ioslides and revealjs among perhaps others) their width and height is 0. This means that when the slide they are hosted on is shown they appear invisible.

Most widgets don't have this problem, but at least one developed by @bwlewis does and it must also also affect others. @bwlewis Could you confirm that this resolves the problems you have been seeing?

@timelyportfolio
Copy link
Collaborator

This is great, but what if we don't have window.jQuery?

Also, sort of related, we discovered an issue with svg and ioslides (see plotly/plotly.R#463).

@jjallaire
Copy link
Collaborator Author

It doesn't need to be written as a jquery plugin that's just how I did it
for dygraphs.

On Sat, Apr 9, 2016 at 9:52 AM, timelyportfolio [email protected]
wrote:

This is great, but what if we don't have jQuery?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#199 (comment)

@jjallaire
Copy link
Collaborator Author

Oh sorry you are talking about the resize thing (not the other issue I
posted). I don't know how to do this quite as cleanly without jquery
(although I'm sure it's possible). The previous workaround called resize on
every widget in the entire slideshow every time a slide was shown/hidden.
The jquery allowed me to easily attach the event handler to just the
slide containing the widget.

Net: at a practical level jquery will be present 98% of the time so this
workaround will take effect in those cases. If someone else wants to write
the equivalent non-jquery code I'm all for it, but if no one has the time
or motivation to do that right now I don't think it should prevent us from
taking this fix in the meantime.

On Sat, Apr 9, 2016 at 10:41 AM, JJ Allaire [email protected] wrote:

It doesn't need to be written as a jquery plugin that's just how I did it
for dygraphs.

On Sat, Apr 9, 2016 at 9:52 AM, timelyportfolio [email protected]
wrote:

This is great, but what if we don't have jQuery?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#199 (comment)

@timelyportfolio
Copy link
Collaborator

On my phone but this http://clubmate.fi/jquerys-closest-function-and-pure-javascript-alternatives/ appears to offer a reasonable alternative to closest. I will volunteer to try this or some other ideas I have so that this will be available without jQuery.

@ramnathv
Copy link
Owner

ramnathv commented Apr 9, 2016

This looks good to me. And I agree with @jjallaire that we should roll with the jquery solution for now and if we end up creating a vanilla js equivalent, we can swap it out for this code. Do we need to test this with a shiny runtime @jcheng5 ?

@jcheng5
Copy link
Collaborator

jcheng5 commented Apr 11, 2016

Hmmm. I don't understand why this code changes anything--the registration right above (to "shown.htmlwidgets") should cover all "shown" events in the document (unless someone is calling event.stopPropagation() on the shown event, which seems unlikely?).

@jjallaire Do you have a repro case I can look at?

@jjallaire
Copy link
Collaborator Author

Indeed I can't repro any longer as it looks to me like the global shown
event is taking care of it. Here's the change where this was fixed (right
before the CRAN release of last May):

b772404

I put the dygraphs code in long before that so for that case this fix is
essentially already in.

However, I was seeing the same problem with revealjs even with that commit.
This change went in after the May CRAN release (and I would have been
testing revealjs without it) so perhaps the revealjs issue was due to this
bug not yet being fixed:

1582c46

So it looks like none of this is necessary except that @bwewis still has
a repro of this in ioslides with one of his widgets. @bwlewis could you
post a simple version of this repro?

On Mon, Apr 11, 2016 at 2:15 AM, Joe Cheng [email protected] wrote:

Hmmm. I don't understand why this code changes anything--the registration
right above (to "shown.htmlwidgets") should cover all "shown" events in
the document (unless someone is calling event.stopPropagation() on the
shown event, which seems unlikely?).

@jjallaire https://github.com/jjallaire Do you have a repro case I can
look at?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#199 (comment)

@bwlewis
Copy link
Contributor

bwlewis commented Apr 11, 2016

Will do, I just got back. I will first make sure I was using latest versions of everything, perhaps that machine had something old by chance. More soon.

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.

5 participants