Skip to content
This repository has been archived by the owner on Feb 3, 2024. It is now read-only.

Load icons from sprite file #3642

Merged
merged 3 commits into from
Jan 5, 2024
Merged

Load icons from sprite file #3642

merged 3 commits into from
Jan 5, 2024

Conversation

jrjohnson
Copy link
Member

Instead of using the fontawesome javascript process we can load our icons out of the sprite file and reference them in components. This requires a larger initial download, however the browser will cache that data. I think we can play some
pre-loading games like we do for fonts to push the sprite files along with the initial page request to improve this even more.

This allows us to drop all of the fontawesome addon and move away from our team needing to work to maintain that addon ourselves and should unblock our move to pnpm.

Instead of using the fontawesome javascript process we can load our
icons out of the sprite file and reference them in components.
This requires a larger initial download, however the browser will cache that data.
I think we can play some
pre-loading games like we do for fonts to push the sprite files along
with the initial page request to improve this even more.

This allows us to drop all of the fontawesome addon and move away from
our team needing to work to maintain that addon ourselves and should
unblock our move to pnpm.
@jrjohnson jrjohnson marked this pull request as ready for review January 4, 2024 19:07
@jrjohnson jrjohnson requested a review from stopfstedt January 4, 2024 19:07
@stopfstedt
Copy link
Member

stopfstedt commented Jan 5, 2024

the original FaIcon component could take a @transform attribute, whereas the drop-in replacement doesnt.

this leads to some (minor) changes that are not intentional.

before

image

after

image

the affected components are:

stefan@nichtsnutz: ~/dev/projects/common on plain-icons
$ grep -r '@transform' addon
addon/components/truncate-text.hbs:      <FaIcon @icon="ellipsis" @transform="down-6"/>
addon/components/editable-field.hbs:                <FaIcon @icon="ellipsis" @transform="down-6" />

Copy link
Member

@stopfstedt stopfstedt left a comment

Choose a reason for hiding this comment

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

see other code comment regarding @transform argument. otherwise, this looks good to me.

@jrjohnson jrjohnson added the run percy tests Activate the visual test runner label Jan 5, 2024
The ellipses fontawesome icon was using the transform property which
requires javascript computations to align correctly. This is the only
place we're using the transform so I manually re-created the effect in a
new component.
@jrjohnson jrjohnson added run percy tests Activate the visual test runner and removed run percy tests Activate the visual test runner labels Jan 5, 2024
@stopfstedt stopfstedt self-requested a review January 5, 2024 17:29
@dartajax dartajax merged commit 24584f7 into ilios:master Jan 5, 2024
19 of 20 checks passed
@jrjohnson jrjohnson deleted the plain-icons branch January 5, 2024 21:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
run percy tests Activate the visual test runner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants