Migrate to font-awesome v5 #3050
Conversation
ghost
added
|
@SaptakS thanks for preparing four well separated commits: makes the life of the reviewer soooo much easier :-) The class modifications is the delicate part to review. To that end it would be great if you could explain (in the commit message) how you proceeded to make sure all v4 class are accounted for. And also, for each class that was replaced how you selected the replacement. |
|
Ran and to visually verify screenshots still look good. I wish I had a way to see the screenshots side by side, before and after. That will be for another time though ;-) |
Codecov Report
@@ Coverage Diff @@
## develop #3050 +/- ##
========================================
Coverage 88.22% 88.22%
========================================
Files 32 32
Lines 1852 1852
Branches 212 212
========================================
Hits 1634 1634
Misses 168 168
Partials 50 50Continue to review full report at Codecov.
|
There was a problem hiding this comment.
It looks good. One thing before we can merge this: install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/files/usr.sbin.apache2 must be updated with the list of all files. Otherwise access will be denied when using them in a hardened environment.
And a comment added to the Font Awesome v5 migrate: Modify classes in the html template file commit with a link to https://fontawesome.com/how-to-use/upgrading-from-4#icon-name-changes-complete-list
|
I still see many occurrences of the fa prefix although the documentation reads Keep in mind you’ll need to change all brand icons from version 4 to us the new fab prefix instead of the old fa prefix. Am I mistaken ? securedrop/journalist_templates/_source_row.html:8: formaction="{{ url_for('col.remove_star', filesystem_id=source.filesystem_id) }}"><i class="fa fa-star"></i></button>
securedrop/journalist_templates/_source_row.html:13: formaction="{{ url_for('col.add_star', filesystem_id=source.filesystem_id) }}"><i class="fa fa-star"></i></button>
securedrop/journalist_templates/_source_row.html:23: <a class="btn small" href="/download_unread/{{ source.filesystem_id }}"><i class="fa fa-download"></i> {{ gettext('{num_unread} unread').format(num_unread=source.num_unread) }}</a>
securedrop/journalist_templates/admin.html:6: <button class="sd-button" type="submit" class="btn" id="add-user"><i class="fa fa-plus"></i>{{ gettext('ADD USER') }}</button>
securedrop/journalist_templates/admin.html:25: <td><a href="/admin/edit/{{ user.id }}" class="plain" data-username="{{ user.username }}"><i class="fa fa-edit" title="{{ gettext('Edit user {username}').format(username=user.username) }}"></i></a></td>
securedrop/journalist_templates/admin_add_user.html:39: <button type="submit" class="sd-button"><i class="fa fa-plus"></i>{{ gettext('ADD USER') }}</button>
securedrop/journalist_templates/col.html:11: <i class="fa fa-chevron-right"></i>
securedrop/journalist_templates/col.html:26: <button type="submit" name="action" value="download" class="small"><i class="fa fa-download"></i> {{ gettext('Download Selected') }}</button>
securedrop/journalist_templates/col.html:38: <span title="Unread" class="icon"><i class="fa fa-envelope"></i></span>
securedrop/journalist_templates/col.html:41: <span class="icon"><i class="fa fa-envelope-open"></i></span>
securedrop/journalist_templates/col.html:53: <i class="fa fa-download"></i> <span class="filename">{{ doc.filename }}</span></a></span>
securedrop/journalist_templates/col.html:59: <i title="{{ gettext('Reply') }}" class="fa fa-reply pull-right"></i>
securedrop/journalist_templates/col.html:92: <h3><i class="fa fa-reply"></i> {{ gettext('Reply') }}</h3>
securedrop/journalist_templates/config.html:15: <i class="fa fa-bell"></i>{{ gettext('SEND TEST OSSEC ALERT') }}
securedrop/journalist_templates/flag.html:4: <i class="fa fa-info-circle pull-left"></i>
securedrop/journalist_templates/index.html:11: <button type="submit" name="action" value="download-unread" class="small"><i class="fa fa-download"></i> {{ gettext('Download Unread') }}</button>
securedrop/journalist_templates/index.html:12: <button type="submit" name="action" value="download-all" class="small"><i class="fa fa-download"></i> {{ gettext('Download') }}</button>
securedrop/journalist_templates/index.html:13: <button type="submit" name="action" value="star" class="small"><i class="fa fa-star"></i> {{ gettext('Star') }}</button>
securedrop/static/js/journalist.js:10: $('div#select-container').html('<span id="select_all" class="select"><i class="fa fa-check-square-o"></i> ' + get_string("select-all-string") + '</span> <span id="select_unread" class="select"><i class="fa fa-check-square-o"></i> ' + get_string("select-unread-string") + '</span> <span id="select_none" class="select"><i class="fa fa-square-o"></i> ' + get_string("select-none-string") + '</span>');
securedrop/static/js/journalist.js:12: $('div#index-select-container').replaceWith('<span id="select_all" class="select"><i class="fa fa-check-square-o"></i> ' + get_string("select-all-string") + '</span> <span id="select_none" class="select"><i class="fa fa-square-o"></i> ' + get_string("select-none-string") + '</span>');
|
|
@dachary that is only for the brand icons. As far as I have checked, we don't use any brand icons. We mostly use solid or regular icons. Solid icons can either have the prefix |
…match with v5 The class names have been updated following the list of changed class names present here: https://fontawesome.com/how-to-use/upgrading-from-4#icon-name-changes-complete-list
Update the list of files in install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/files/usr.sbin.apache2 to prevent access denied in hardened environments.
SaptakS
force-pushed
the
migrate/font-awesome-v5
branch
from
7b9829a to
b548db5
Compare
|
@SaptakS in order to verify no class was left behind, will I be exhaustive in my review if I do the following: to see all lines with a fontawesome class that could be in the list of name to change, extract each name, make a list, remove duplicates which leads to: and then verify for each of them they are converted as suggested The fa-check-square-o is in js files, reason why you missed it. What about the ones I did not find ? Does that mean they stay as they are ? |
|
@dachary yes. That is what they mean. If it is not there in the change list, it works with the old class. I will still re-check to confirm I am not missing anything. And I will fix the |
The class names have been updated in the js files following the list of changed class names present here: https://fontawesome.com/how-to-use/upgrading-from-4#icon-name-changes-complete-list
|
Visually verified the classes changed by Font Awesome v5 migrate: Update js files with the new class names work as advertized by:
|
|
the CircleCI failure looks like a transient error, like the machine breaking down. I'm not sure why anything in this PR would make tests hang for more than 10 min doing nothing. https://circleci.com/gh/freedomofpress/securedrop/8957?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link TASK [Run application tests] ***************************************************
Thursday 22 February 2018 13:10:37 +0000 (0:00:00.037) 0:01:07.570 *****
Makefile:20: recipe for target 'ci-go' failed
make: *** [ci-go] Terminated
Too long with no output (exceeded 10m0s)
|
|
Hum, browsing the source directory I found @SaptakS relevant PRs could be #1567 and #1480 but I did not look into them. |
|
Ah that icon is used by the source interface, it does not need to be updated. For context, back when we started recommending sources to turn the Tor Browser security slider to its safest setting (#1480), we knew that all Font Awesome icons would not render (as this would be disabled by Tor Browser), so we replaced the Font Awesome icons used on the source interface with identical PNGs (#1567). In some files - like the one you point out - the "fa" substring from the Font Awesome icon name may still be there. We can always change the name for clarity if it helps |
It makes sense now, thank you. And I see the |
|
Verified that include static/fonts/ include static/fonts/** meaning all files from the fonts directory will be packaged, not a selection that would need to be updated. |
|
I tested in the staging environment, it would be great if you could do the same @SaptakS to verify all works as it should in an environment that is almost as production (modulo SSH access but that's entirely unrelated to this PR). Instructions on how to setup a staging environment are at https://docs.securedrop.org/en/latest/development/virtual_environments.html#staging-vms but I'd be happy to answer any questions you may have. You will need a to cherry-pick the two commits from #3062 otherwise it won't work. They are not related to this PR, the problem existed before. |
|
@dachary ok. will check in the staging environment |
|
@SaptakS in case that helps you'll find in another post detailed instructions on how to setup a staging environment. |
|
Hi @SaptakS - this looks great. One last thing prior to merge: can you provide a link to where the webfonts files in this PR were downloaded from? |
There was a problem hiding this comment.
Marking as changes requested for now - just this question to address
|
@redshiftzero the font files came with the zip downloaded from here |
|
Are you sure v5.0.7 is the correct version? Here are the SHA256 hashes of the font files in this PR: and in the v5.0.7 Font Awesome download: |
|
@redshiftzero sorry. Seems the one I used in this PR was v5.0.6 and they updated to v5.0.7 after that. I will update to v5.0.7 if need be. Sorry for the inconvenience. Didn't notice they have pushed in a new release. |
There was a problem hiding this comment.
Confirming these font files are from v5.0.6 of Font Awesome. Thanks @SaptakS!
|
No need to update, I just wanted to cross-check the hashes prior to merge. All looks good! |
Status
Ready for review
Description of Changes
In this PR, I have made changes in the font files, the font-awesome sass file and changed the classes in the templates as required.
One of the reasons for doing this is using the updated globe icon of font-awesome v5 in the language dropdown.
Testing
How should the reviewer test this PR?
Check for the icons in the journalist and source interface.
Checklist
If you made changes to the app code: