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.

Sign up for GitHub

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

Jump to bottom

DEV - Add Fontawesome scripts to build assets #1982

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

DEV - Add Fontawesome scripts to build assets #1982

wants to merge 5 commits into from
+9 −8

Conversation

trallard
Copy link
Collaborator

I seem to have forgotten to add the FA scripts to the pyproject.toml so I am adding it here (which should fix the FA rendering issue in the dev version of the docs).

@gabalafou Additionally, there seems to be a new issue: Uncaught ReferenceError: FontAwesome is not defined at... where we add the custom PyData and PyPI icons. However, I do not know how to resolve this (this is beyond my JS knowledge).

There is this document on customizing https://github.com/FortAwesome/Font-Awesome/wiki/Customize-Font-Awesome, which might be helpful for you. Could you please have a look?

@trallard trallard added tag: dependencies Pull requests that update a dependency file tag: javascript Pull requests that update Javascript code labels Sep 12, 2024
@trallard trallard marked this pull request as draft September 12, 2024 17:10
@github-actions GitHub Actions
Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

@gabalafou
Copy link
Collaborator

Related:

@gabalafou
Copy link
Collaborator

I can explain your Uncaught ReferenceError and offer a way to solve it.

We are dealing with a dependency failure between two scripts:

  1. fontawesome.js
  2. custom-icon.js

The Font Awesome library needs to be loaded first because the custom icon script calls it like so:

FontAwesome.library.add(/* icon object */)

The Font Awesome and custom icon scripts are loaded with different Jinja macros at different points in the layout template (which is the base template for all of the pages in the docs). Going from the top to the bottom of the template, here is a list of the relevant macros:

  1. head_pre_icons() - prior to PR 1955, this macro loaded the Font Awesome script. It is located in the HTML <head>.
  2. script() - Sphinx defines this macro. It also defines the Jinja {% block scripts %} and calls the macro within this block. Sphinx puts this block in the HTML <head> and we made sure to call the head_pre_icons() macro before this macro.
  3. body_post() - after PR 1955 (and at the time of this writing), this macro loads the Font Awesome script. It is located near the end of the </body>.

So this is the problem. The custom icon script depends on the Font Awesome script. Before PR 1955, the Font Awesome script was loaded before the custom icon script. Now it's loaded after. So when the custom icon script executes, it references an undefined global, FontAwesome, and throws an uncaught error.

There are a number of different ways we could try to solve this, but the most straightforward is to restore the head_pre_icons() macro and load the Font Awesome script there like before. That's what I think we should do in the short-term even it undoes the performance gain of moving the Font Awesome script tag to the bottom of the page. We can then take some time to think more carefully about how to optimize the page load.

Note that PR 1955 also added the defer attribute to the Font Awesome script tag. We also have to undo that because defer has an effect that is similar to moving the script tag to the bottom of the page.

@trallard trallard marked this pull request as ready for review September 18, 2024 14:35
@trallard
Copy link
Collaborator Author

Thanks @gabalafou to fix the immediate issues with FA I added the FontAwesome JS scripts to the <head> and will look into further improvements later on

@trallard
Copy link
Collaborator Author

Will also mark this as a release blocker as we need to get this merged to ensure that FA is properly vendored for end-users

@trallard trallard added the impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship label Sep 18, 2024
@gabalafou gabalafou mentioned this pull request Sep 19, 2024
Open
@trallard trallard changed the title WIP - Add Fontawesome scripts to build assets DEV - Add Fontawesome scripts to build assets Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabalafou gabalafou Awaiting requested review from gabalafou

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship tag: dependencies Pull requests that update a dependency file tag: javascript Pull requests that update Javascript code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trallard @gabalafou