dspace-angular
dspace-angular copied to clipboard
Font Awesome Angular library (angular-fontawesome)
I propose to use @fortawesome/angular-fontawesome instead of @fortawesome/fontawesome-free
https://www.npmjs.com/package/@fortawesome/angular-fontawesome
This library provides an Angular component that allows to add icons like this...
<fa-icon [icon]="['fas', 'fa-music']"></fa-icon>
...instead of this:
<i class="fas fa-music"></i>
The advantage of this library is that it provides the Auto Accessibility feature.
In order to make current icons accessible, without the alternative libraty, we would need to add either a title attribute or aria-hidden="true" to each icon.
The library can be used alongside the current one, so we won't need to migrate all the icons in one shot.
For what it's worth, this seems reasonable to me @davide-negretti , especially since @fortawesome/angular-fontawesome seems to be an official Angular version from the same team that provides Font Awesome.
Personally, though, if possible, I'd recommend changing over all at once. I'd rather not keep two similar dependencies around for very long, as it encourages developers creating new PRs to continue to use @fortawesome/fontawesome-free.
In any case, this needs a volunteer.
Hi, I want to ensure that we're all aligned on the changes that need to be implemented. Could you @tdonohue please let me know if you're already aware of these proposed modifications? I've detailed the necessary changes below for your convenience.
At present, we're utilizing @fortawesome/fontawesome-free. In the src/styles/helpers/_font_awesome_imports.scss file, we import four different SCSS files associated with FontAwesome. This method employs FontAwesome's font approach, which allows us to use icons in a straightforward manner, like so:
<em class="fas fa-search fa-lg fa-fw"></em>
The above code is rendered as:
<em _ngcontent-dspace-angular-c347="" class="fas fa-search fa-lg fa-fw ng-tns-c347-7"></em>
However, we're considering migrating to @fortawesome/angular-fontawesome, which uses an SVG approach instead of fonts. This requires us to register the FontAwesomeModule and each icon we intend to use, as shown below:
@NgModule({
declarations: [
AppComponent,
],
imports: [
...IMPORTS,
FontAwesomeModule,
],
providers: [
...PROVIDERS,
],
bootstrap: [AppComponent],
})
export class AppModule {
constructor(library: FaIconLibrary) {
library.addIcons(faSearch); // Each icon we want to use in the application should be registered here
}
......
}
With this setup, we can use a component-based approach to add an icon in the template:
<fa-layers [fixedWidth]="true">
<fa-icon [icon]="['fas', 'search']" size="lg"></fa-icon>
</fa-layers>
The above code is rendered as:
<fa-layers _ngcontent-dspace-angular-c347="" class="ng-tns-c347-7 fa-layers fa-fw" ng-reflect-fixed-width="true">
<fa-icon _ngcontent-dspace-angular-c347="" size="lg" class="ng-fa-icon" ng-reflect-size="lg" ng-reflect-icon="fas,search">
<svg role="img" aria-hidden="true" focusable="false" data-prefix="fas" data-icon="magnifying-glass" class="svg-inline--fa fa-magnifying-glass fa-lg" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512">
<path fill="currentColor" d="M416 208c0 45.9-14.9 88.3-40 122.7L502.6 457.4c12.5 12.5 12.5 32.8 0 45.3s-32.8 12.5-45.3 0L330.7 376c-34.4 25.2-76.8 40-122.7 40C93.1 416 0 322.9 0 208S93.1 0 208 0S416 93.1 416 208zM208 352a144 144 0 1 0 0-288 144 144 0 1 0 0 288z"></path>
</svg>
</fa-icon>
</fa-layers>
Let me know if you have any questions.
This looks like a significant improvement in accessibility. Yes, please.
The need to declare each icon is a minor inconvenience for developers, but also provides a place to see which icons are already in use when pondering how to pictorialize a UI feature, as well as making the code easier to understand.
@pcg-kk : At a glance that sounds reasonable. However, I do want to note that I'm not sure whether we'll have time to add this into DSpace 8.0. We have already passed the Feature deadline and our 8.0 Testathon starts next week. See the 8.0 release timeline.
If the scale of the changes here are very large (and they sound like they might be), then it may be difficult to get them tested/approved for the 8.0 final release. That said, we can do our best. If it doesn't make it into 8.0, then the developer team can discuss how best to move this forward and whether it could go into a 8.1 or if it needs to wait for 9.0.
In any case, you are welcome to start working on this, and I can find volunteer reviewers once it is ready for review/testing.
All occurrences of FontAwesome icons are replaced by a new library except for the dynamically declared icons. I would like to propose splitting those dynamics into a separate PR because it will be easier for the reviewer to check all "just replacements" and then the logical changes. WDYT @tdonohue ?
If you accept this idea to split this task into 2 separate ones, I can prepare the next one immediately after merging this one. Or if you prefer to remove the old font-awesome completely in this one task & PR I need one day more to finish it.