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

Do not load external bootstrap resources for cdn_resources = 'in_line' | 'local' #201

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

FieteO
Copy link

@FieteO FieteO commented Dec 28, 2022

The cdn_resources option should control how external resources and stylesheets should be loaded.
Currently bootstrap is being fetched from CDN irrespective of the cdn_resources option.

Also I have observed some inconsistencies in the local option where the vis-network css and js files are not being taken from the lib folder but rather externally fetched. Please correct me here if my understanding of how these options are intended to work is wrong.

To summarise the changes:

  • fetch bootstrap from CDN when cdn_resources = 'remote'
  • use resources from lib/ when cdn_resources = 'local'
  • remove the double heading (which is also being fixed in Removed duplicated heading in HTML output. #191)
  • run example.ipynb to update the samples

@BerserkerGaruk BerserkerGaruk self-assigned this Jan 12, 2023
Copy link
Member

@BerserkerGaruk BerserkerGaruk left a comment

Choose a reason for hiding this comment

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

Went through the commit, I agree with the changes made, but I'm going to need to verify the hash between the bootstrap and visj min.js and .min.css files on my side before merging.

@FieteO
Copy link
Author

FieteO commented Jan 19, 2023

Do you need anything from my side for validating the checksums? @BerserkerGaruk

@FieteO FieteO force-pushed the do-not-load-external-resources-for-in-line branch from f8736bc to 19b8f78 Compare March 14, 2023 21:18
@FieteO
Copy link
Author

FieteO commented Mar 14, 2023

@BerserkerGaruk @summerswallow-whi , I have rebased the branch to resolve the merge conflicts that occurred in the meantime. I would appreciate it, if you could take a look, since the MR is open for a couple of month now

@fohrloop
Copy link

fohrloop commented May 3, 2023

Is there anything that could be done to help with this?

Copy link

@wey-gu wey-gu left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this fix!

@wey-gu
Copy link

wey-gu commented Apr 1, 2024

I had verified e2e this change on my offline code with in_line, it worked like a charm

cc @summerswallow-whi @BerserkerGaruk

Thanks!

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.

4 participants