LibreTexts / ckeditor-binder-plugin

A CKeditor plugin that makes adding binder enabled pre tags easy.
GNU General Public License v3.0
4 stars 2 forks source link

FontAwesome V5 #157

Closed Miniland1333 closed 3 years ago

Miniland1333 commented 3 years ago

Currently, this plugin is using the older FontAwesome v4 instead of the newer v5. The newer version has a significant rewrite under the hood and contains new icons that Delmar wants to use for other things.

FontAwesome v4 is currently being loaded within ckeditor-blinder-plugin at: https://github.com/LibreTexts/ckeditor-binder-plugin/blob/staging/src/scripts/registerPlugin.js#L13

Since FontAwesome is also shared by a few other pieces of code/HTML, do you think that it would be a good idea to make FontAwesome an external dependency rather than the current method of loading it within this plugin?

sandertyu commented 3 years ago

I believe we could do that for libretexts.org. The fontawesome script is necessary for many jupyterlab widgets to load properly, so if we remove it from the plugin then the plugin will not work the same when standing alone. Then again, we can probably just manually include the font awesome script on any html page we test it with that isn't libretexts.org.

Are you suggesting we remove the dependency from the plugin entirely, and just use the one provided by libretexts.org? We can probably at least try that.

Miniland1333 commented 3 years ago

Yes, that does sound like what I would like to try for staging. One thing for us to remember is that we will likely need to use the v4 shims to make sure the plugins don't experience any compatibility issues. Let me know if you need help testing this on staging/Query and this is not urgent. https://cdnjs.com/libraries/font-awesome

moorepants commented 3 years ago

The plugin should work outside of libretexts.org too. So, however you set this up, it would need to skip loading things if on that particular website.

sandertyu commented 3 years ago

I have removed font-awesome from src/scripts/registerPlugin.js and included some additional scripts to the src/index.html for independent testing. This is on staging and has been deployed to query. Removing font-awesome seems to have mostly not effected anything, but this widget is not displaying the proper button symbols on both libretexts and in our development page;

matplotlib

moorepants commented 3 years ago

I don't think this is a good idea, in general.

Miniland1333 commented 3 years ago

https://query.libretexts.org/Sandboxes/hdagnew@ucdavis.edu/Jupyter_Widgets

image

Okay so I was able to get FontAwesome v5 working with the shim.

moorepants commented 3 years ago

I'm no longer seeing the icons on the buttons on query: image

https://query.libretexts.org/Sandboxes/jupyterteam_at_ucdavis.edu

moorepants commented 3 years ago

image

moorepants commented 3 years ago

The matplotlib example on query.libretexts.org now works and has the icons on the buttons.

sandertyu commented 3 years ago

We have merged in the changes suggested by Henry to load fontawesome based on an if statement which checks for certain tags within the HTML of the webpage. However, fontawesome does not load on its own when just using the local development version. We'll want to figure out why this is happening, and how to bundle fontawesome with this plugin regardless of context.

moorepants commented 3 years ago

@Miniland1333 it doesn't seem your fixed worked for the versions not loaded on libretexts. Can you have a look?

Miniland1333 commented 3 years ago

If you are referring to how the changes aren't showing up when using yarn serve, my guess is that the different webpack configs have different entry-points. When using prod, it uses registerPlugin.js which includes these changes, but during development it uses index.js which does not.

This is evidenced by the following config that is only in webpack-production:

  entry: {
    registerPlugin: Path.resolve(__dirname, '../src/scripts/registerPlugin.js'),
  },
moorepants commented 3 years ago

Thanks Henry. With the change you made, font awesome should be loaded by the plugin in production and development environments (if not on libretexts).

Miniland1333 commented 3 years ago

I don't have time to test it, but you should be able to copy over the lines involving the loadScript() from registerPlugin.js to index.js so that it activates in both production and development.

TimStewartJ commented 3 years ago

I'll go ahead and close this, we can use #171 to use FA5 in local environments. I think its a better idea to keep the loading in registerPlugin.

Miniland1333 commented 3 years ago

Closed by #171