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

update fontawesome version #438

Closed
wants to merge 1 commit into from

Conversation

drammock
Copy link
Collaborator

@drammock drammock commented Jul 29, 2021

I recently had to recreate my conda envs from scratch, and when I ran setup for pydata-sphinx-theme, it pulled in a newer version of fontawesome (5.15.3 instead of 5.13.0). This PR is the result of yarn build:production followed by pre-commit run --all-files and nothing else.

An alternative would be to update package.json to freeze fontawesome at a specific version (current spec is "@fortawesome/fontawesome-free": "^5.13.0"). Which do you prefer?

NB: I also had to specifically downgrade my installed version of node in order to be compatible with node-sass... requested version is "node-sass": "^4.13.1" which IIUC will only pull in 4.x versions. This limits users to node 14; node 15 requires node-sass 5.x, node 16 requires node-sass 6.x (source: https://www.npmjs.com/package/node-sass). Not sure what the right fix is there.

@drammock
Copy link
Collaborator Author

hmm, a lot of CI failures. I'm not very familiar with your test suite and whether failures would be expected for a dependency-updating PR, so I welcome core-dev help in sorting out what is going wrong (looks like several things: a missing file, unexpected diffs, low accessibility scores...)

@jorisvandenbossche
Copy link
Member

An alternative would be to update package.json to freeze fontawesome at a specific version

We have some docs about updating a font version: https://pydata-sphinx-theme.readthedocs.io/en/latest/contributing.html#upgrading-a-font. This mentions to update the version with yarn, which I suppose will also update the package.json with the new version (and it is also followed by a yarn build:production).

NB: I also had to specifically downgrade my installed version of node in order to be compatible with node-sass... Not sure what the right fix is there.

I suppose we could update the node-sass version?

hmm, a lot of CI failures.

I am fixing the unexpected diffs in #441. The missing file is I think an actual problem with this PR.

@drammock
Copy link
Collaborator Author

drammock commented Aug 9, 2021

We have some docs about updating a font version

Thanks, I missed that before. Reading that now, IMO it would actually be better to pin the font version. It seems odd to me that a new contributor might clone the repo and follow the recommended setup guidance, and immediately be faced with a whole bunch of changes to (tracked) font files that they didn't try to change themselves. I'm not trying to update the fonts to get access to any specific new icon or feature, I just had no choice because of how the dependency was specified. That seems bad.

we could update the node-sass version

That seems right to me (preferable to pinning users to an older version of node)

@drammock
Copy link
Collaborator Author

closing as the correct (or certainly the easier) fix is probably to pin the font version rather than updating the font files

@drammock drammock closed this Aug 13, 2021
@drammock drammock deleted the update-fontawesome branch September 10, 2021 19:20
@drammock drammock mentioned this pull request Dec 28, 2021
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.

None yet

2 participants