Fix broken icons #1768
Merged
Fix broken icons #1768
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- fixes broken icons across the site - icons are glyphicons from bootstrap, which are included as part of that dependency and can't be directly modified - problem is caused by the compiled Sass not being able to find and download the icon font files e.g. https://content-tagger.publishing.service.gov.uk/assets/content-tagger/bootstrap/glyphicons-halflings-regular.eot - this appears to be because the syntax in bootstrap for defining these files uses `url(font-path(path))` which doesn't seem to work anymore, could be related to the recent dart-sass migration but unclear - as a workaround have copied the font-face declaration from the bootstrap CSS into our own, and replaced the above with `url(path)`, which works - this occurs after the bootstrap declaration, overriding it and allowing the icon font file to be found and loaded correctly
mtaylorgds
reviewed
Aug 8, 2024
There was a problem hiding this comment.
Looking at integration, this seems to have fixed some of the icons, but not all of them. For example looking at this page I can see that the double-headed arrow on the right of the drop down now appears, but the X on the left hand side still does not:
andysellick
force-pushed
the
fix-icons
branch
2 times, most recently
from
3685e47
to
a4b6a86
Compare
- these icons appear on the edit a page page and should be on the left of things that can be added or removed from a page - can't be fixed by the previous change as this isn't a glyphicon, for some reason, is part of a dependency so can't directly modify the now not-compiling-in-dart-sass image page - easiest thing is to switch to a glyphicon instead - adding some custom styles to position it properly, have to make it this specific as there's an override to float all glyphicons in this element to the right, where there's already an icon - also adding a JS class for the JavaScript hook
|
Think I've fixed all the icons now. Unfortunately they're mostly in external dependencies, which aren't fully compatible with the new dart-sass compilation, meaning we can't access that code to fix it. I've had to add some unfortunately quite specific overrides in our our Sass, the problem is around background images originally written using |
mtaylorgds
approved these changes
Aug 9, 2024
andysellick
added a commit
to alphagov/transition
that referenced
this pull request
Aug 12, 2024
- relying upon glyphicons as part of bootstrap, but the Sass for that doesn't compile properly in dart-sass because it uses `url(font-path(path))` - instead copy the font-face declaration from bootstrap into our code, overridding the original with this one, which fixes the icon paths and the icons - see similar problem fixed in content-tagger: alphagov/content-tagger#1768
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What / why
url(font-path(path))which doesn't seem to work anymore, could be related to the recent dart-sass migration but unclearurl(path), which worksVisual changes
Before
After
Trello card: https://trello.com/c/gW2NW1sB/239-fix-sass-compilation-warnings