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

use Font Awesome fonts for icons #1319

Merged
merged 2 commits into from
May 4, 2022
Merged

use Font Awesome fonts for icons #1319

merged 2 commits into from
May 4, 2022

Conversation

osma
Copy link
Member

@osma osma commented May 3, 2022

Reasons for creating this PR

The "copy to clipboard" icon was changed to a SVG image derived from the Font Awesome Free icon set in PR #1309. But picking just a single item of the set into a SVG file turned out to be cumbersome especially for Skosmos widgets, which may need many such icons. So this PR changes the way we use Font Awesome, switching to the official Web Fonts+CSS method which means including the whole icon set as a Web Font and then using CSS rules to show individual icons. The same icons may then easily be used by Skosmos widgets as well.

Link to relevant issue(s), if any

Description of the changes in this PR

  • include Font Awesome font and CSS files (minimal selection that only contains the Regular icon set)
  • include the CSS files in the HTML HEAD section
  • replace the copy to clipboard SVG icons with a <span> element that shows the corresponding icon via CSS classes
  • adjust CSS rules for better layout of the clipboard icon

Known problems or uncertainties in this PR

n/a

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma added this to the 2.15 milestone May 3, 2022
@osma osma self-assigned this May 3, 2022
@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #1319 (4a780c2) into master (a54f285) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1319   +/-   ##
=========================================
  Coverage     70.36%   70.36%           
  Complexity     1649     1649           
=========================================
  Files            32       32           
  Lines          3790     3790           
=========================================
  Hits           2667     2667           
  Misses         1123     1123           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a54f285...4a780c2. Read the comment docs.

@osma osma changed the title use Font Awesome fonts for icons, including copy to clipboard icon use Font Awesome fonts for icons May 3, 2022
height: 1em;
vertical-align: text-top;
.prop-preflabel .copy-clipboard {
font-size: 20px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only downside of fixed pixel size is for users that need to change page zoom,as the icon does not scale.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that applies nowadays. CSS pixels are not the same as screen pixels, and when you zoom in on a web page, browsers will happily adjust text sizes even if they are declared in px.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested this using Firefox and Chromium. The icon scales along with the text in both browsers when you zoom in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, when you zoom in in your browser that works. But vision impaired users normally have different font-sizes configured in their browsers.

In that case the web application using pixels is not always rendered correctly.

To test it with Chromium, for instance, go to Settings / Appearance. Then change Font Size that should be set to its default (24) to something like Huge 72 just to test.

image

image

In the GIF below, I'm showing SKOSMOS after changing the browser font size to Huge. Notice that the icon and the text are using the pixel size, ignoring that I've changed the font size (i.e. not relative like em or rem). Then I'm changing the font size to 1em in the icon, and then 1em for the CSS root variable.

GIFrecord_2022-05-04_232059

There are other cases where the fixed pixel size doesn't work or gets in the way of users that with different configurations, but I remember this one because I use a large font size in some devices — family history of poor vision, so I've already learned how to use a screen reader, and try to promote WCAG 2.1/Section 508 (US)/Web Accessibility Standard (NZ)/good practices/etc whenever I can 😬
I tried using em during the Bootstrap 5 work, but the final result was different than Bootstrap 3, so it was simpler to just copy the values from Bootstrap 3. I think for now we should use this 20px, and if we need to support accessibility standards like WCAG we can create a new issue later and see what's required (I think Europe adopted WCAG 2.1 too, but I only worked with a11y in NZ & Brazil).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, thanks for the clarification and example. I first thought that you were commenting on the pixel value used in this PR just for a single icon, but this is a much more general issue which affects many elements in the Skosmos UI.

In fact we do have a requirement to follow WCAG level AA, or at least to document any deviations from it (there are many known a11y problems - and likely many unknown - in current Skosmos, unfortunately). There is an accessibility statement for Finto - in Finnish only - which contains a long list of known problems as well as aspects that have not been verified.

The best solution would probably be to avoid px sizes for fonts altogether. But implementing that should be planned in an issue and implemented as a PR (or several).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed! 👍 😀 Thanks @osma

@osma
Copy link
Member Author

osma commented May 4, 2022

Turns out that we need the Font Awesome Solid set as well for some Skosmos widgets, so I will need to adjust this PR to include that as well.

@sonarcloud
Copy link

sonarcloud bot commented May 4, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@osma osma requested a review from joelit May 4, 2022 12:26
@osma osma merged commit a5ef6c8 into master May 4, 2022
@osma osma deleted the use-fontawesome-free-icons branch May 4, 2022 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants