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

Improved handling of JS and CSS dependencies #426

Merged
merged 11 commits into from Feb 1, 2016

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

philippjfr commented Jan 28, 2016

This PR is an attempt at making the handling of JS and CSS dependencies a bit more sane to allow exporting of notebooks and individual widgets more easily and ensure sufficient control over which libraries are included.

@philippjfr philippjfr added this to the v1.4.2 milestone Jan 28, 2016

js_dependencies = Renderer.js_dependencies + CDN.js_files

css_dependencies = Renderer.css_dependencies + CDN.css_files
backend_dependencies = {'js': CDN.js_files, 'css': CDN.css_files}

This comment has been minimized.

@jlstevens

jlstevens Feb 1, 2016

Contributor

Definitely cleaner. I assume you'll collect the core renderer dependencies together later...

'underscore': {'js': ['https://cdnjs.cloudflare.com/ajax/libs/underscore.js/1.8.3/underscore-min.js']},
'require': {'js': ['https://cdnjs.cloudflare.com/ajax/libs/require.js/2.1.20/require.min.js']},
'bootstrap': {'css': ['https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css']}}

This comment has been minimized.

@jlstevens

jlstevens Feb 1, 2016

Contributor

Looks good. I suspect it might have been possible to avoid the deep nesting with extra_js_dependencies and extra_css_dependencies. Not a major issue so only update it if you want to.

This comment has been minimized.

@philippjfr

philippjfr Feb 1, 2016

Author Contributor

Would also need core_js_dependencies and core_css_dependencies in that case.

This comment has been minimized.

@jlstevens

jlstevens Feb 1, 2016

Contributor

Ok, I have no real objection to leaving it as it is.

js_html += '\n<script type="text/javascript">%s</script>' % widgetjs
css_html += '\n<style>%s</style>' % widgetcss

return js_html, css_html

This comment has been minimized.

@jlstevens

jlstevens Feb 1, 2016

Contributor

Looks good. If necessary, could one day be made to spit out a dictionary of resource locations if we ever needed them without the surrounding HTML tags.

This comment has been minimized.

@philippjfr

philippjfr Feb 1, 2016

Author Contributor

Might as well just access the core and extra dependencies directly.

@@ -32,7 +32,7 @@
fileref.setAttribute("href", filename)
document.getElementsByTagName("head")[0].appendChild(fileref)
}
loadcssfile("https://code.jquery.com/ui/1.10.4/themes/ui-lightness/jquery-ui.css");
loadcssfile("https://code.jquery.com/ui/1.10.4/themes/smoothness/jquery-ui.css");

This comment has been minimized.

@jlstevens

jlstevens Feb 1, 2016

Contributor

Change of theme?

This comment has been minimized.

@philippjfr

philippjfr Feb 1, 2016

Author Contributor

Yea, the slide bar on this one is white, gives a bit more contrast.

@@ -102,7 +102,8 @@
} else {
var dim_val = vals[ui.value];
}
$('#textInput{{ id }}_{{ widget_data['dim'] }}').val(dim_val);
console.log((dim_val*1).toString())

This comment has been minimized.

@jlstevens

jlstevens Feb 1, 2016

Contributor

Is this a stray console.log or a useful one? If it is useful, I assume this won't be called too often.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 1, 2016

Looks good to me as soon as you address the comments I've made.

Philipp Rudiger Philipp Rudiger
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Feb 1, 2016

Thanks for the fix. Merging.

jlstevens added a commit that referenced this pull request Feb 1, 2016

Merge pull request #426 from ioam/jsdependencies
Improved handling of JS and CSS dependencies

@jlstevens jlstevens merged commit 82fe5cf into master Feb 1, 2016

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 69.783%
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens deleted the jsdependencies branch Feb 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.