Skip to content

Conversation

@Blendify
Copy link
Member

@Blendify Blendify commented Feb 27, 2021

This is still a work in progress

  • fix font file not being found

This is still a work in progress, I am havign issues getting the font coppied over
@Blendify Blendify requested review from agjohnson and stsewd February 27, 2021 05:16
"build": "webpack --config webpack.prod.js",
"preinstall": "bin/preinstall.js"
},
"dependencies": {},
Copy link
Member Author

Choose a reason for hiding this comment

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

npm doesnt seem to like this being left blank, it got removed automatically

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

yay! we may hold-off merging this to plan for a 0.6.0 release

@stsewd stsewd added the Status: blocked Issue is blocked on another issue label Mar 1, 2021
@Blendify
Copy link
Member Author

Blendify commented Mar 1, 2021

I was having a hard time getting this to work without having to set the font face for .fas it doesn't seem like this should be necessary, it wasn't before and I don't see anything in the FA docs about this so maybe I am doing something wrong.

@Blendify Blendify added this to the 1.0.0 milestone Mar 11, 2021
Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Going to block on renaming the fas class on this PR. I think we should figure out how to reuse fa class here, to avoid another breaking change for downstream packages.

+font-face(FontAwesome, '#{$fa-font-path}/fa-solid-900')

.fa:before
.fas
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to keep this fa or this is a breaking change as well. Downstream projects could be relying on fa classes and the rename seems easy enough to avoid

@polyzen
Copy link

polyzen commented Mar 12, 2021

Fork Awesome activity seems have stalled since October, but something to consider:
https://forkaweso.me/Fork-Awesome/whats-new/
FortAwesome/Font-Awesome#12199 (comment)

@agjohnson I don't know how feasible using fa- with FA 5 is, but Fork Awesome still uses fa-.

@Blendify
Copy link
Member Author

Yeah I don't think trying to keep the use of fa- is a good idea, It makes it confusing for developers to know if we are using fa5 when we make a hack to continue using fa4 syntax.

I also don't think using a fork of fa is a good idea either, this will only add headaches when the fork is no longer maintained which seems to already be the case.

@agjohnson
Copy link
Collaborator

There are font awesome 4 shims available for font awesome solid. This would be the best intermediate step without cutting a backwards incompatible release.

If we keep the font awesome solid prefix, this is a strong breaking change for downstream users and we're likely already then talking about a v2.0 release with this change. v1.0 is shaping up to be dropping Sphinx <1.6 support.

@agjohnson agjohnson modified the milestones: 1.0, 2.0 Mar 17, 2021
@agjohnson
Copy link
Collaborator

I still feel that this is an easy backwards incompatible change to avoid and that we should add the FA4 class naming to provide some small amount of insurance here. The end user can always customize on top of our theme, and could be expecting these classes that the theme has been providing. It will be difficult to catch this or warn the user about this, so I lean towards avoiding this scenario, especially with the number of points of backwards incompatibility that we already have.

This only requires defining the additional class name to set the font-family and font-weight. Here's an example of defining the classes manually:

https://github.com/readthedocs/sui-common-theme/blob/236e6b12ae93d99321cf930fbb567d7964d5ea36/elements/icon.overrides#L7-L26

I'll include this in a later milestone for now.

@agjohnson agjohnson modified the milestones: 2.1, 2.2 Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: blocked Issue is blocked on another issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants