Fix #2348, #2329, #1817: bugs triggered by networkD3 sankey plot #2361
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@wch One problem with this approach that we may want to rectify is, if the bootstrap-datepicker repo disappears, we don't have a way to revert to a "clean import", since the version in git is the one we've patched. We might consider adding the bootstrap-datepicker release zip file to our repo as part of the import process (for 1.6.4 it's 97k). What do you think? Update Nevermind, I realized that if we ever needed to do this we could |
wch
requested changes
Mar 27, 2019
tools/README.md
Outdated
|
|
||
| After updating, our patches to `bootstrap-datepicker` must be applied using the script `applyDatepickerPatches.R` | ||
|
|
||
| After updating and applying patches, `yarn grunt` should be run per the instructions above. |
There was a problem hiding this comment.
Might want to mention why this is necessary (to generate the minified JS file).
tools/README.md
Outdated
|
|
||
| 1. Make any necessary changes to files in `inst/www/shared/datepicker` | ||
| 1. **Do not commit your changes.** | ||
| 1. Instead, create a patch with a command like `git diff > tools/datepicker-patches/012-a-description.patch` |
There was a problem hiding this comment.
Mention that the patches are applied in numeric order.
tools/updateBootstrapDatepicker.R
Outdated
| unzip( | ||
| dest_file, | ||
| files = c( | ||
| "js/bootstrap-datepicker.js", |
alandipert
force-pushed
the
fix-svg-foreignobject
branch
3 times, most recently
from
99715d6 to
a13dd73
Compare
* All of these were caused by the presence of multiple body tags on the page, which happened because networkD3's sankey plot generates SVGs containing body tags via SVG's foreignObject tag * In various places, the 'body' jQuery selector string is used under the assumption there is only one 'body' tag on the page. The presence of multiple 'body' tags breaks reliant code in strange ways. * The fix was to use document.body or 'body:first' instead of 'body'.
alandipert
force-pushed
the
fix-svg-foreignobject
branch
from
a13dd73 to
908d635
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR supersedes #2359 as it includes some new infrastructure for managing patches to
bootstrap-datepickerCause and fix
'body'jQuery selector string is used under the assumption there is only onebodytag on the page. The presence of multiplebodytags breaks reliant code in strange ways.'body:first'ordocument.bodydo the job.Issues that were fixed
Further context
Note that this has troubled networkD3 users for a long time: christophergandrud/networkD3#214
The way Shiny behaves, that this PR addresses, is not strictly "wrong" — it should be safe to assume that in a given HTML page, only one
bodywill appear. However, the existence of the problem is proof of how confusingforeignObjectis (the SVG spec example forforeignObjectdemonstrates embeddingbodyin an SVG!), and so it seems like a good policy for Shiny to do the robust (if technically unnecessary) thing.New patch infrastructure
This PR also includes two new scripts,
tools/updateBootstrapDatepicker.Randtools/applyDatepickerPatches.R, that can be used together to manage our own changes to thebootstrap-datepicker3rd party library. A section was added totools/README.mddocumenting their use.In order to switch to this new method of applying patches, bootstrap-datepicker version 1.6.4 was re-imported, and a previously committed change was converted to a
.patchand re-applied using the new tools.