-
Notifications
You must be signed in to change notification settings - Fork 688
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
Add globe icon in the locale dropdown for better understanding #3121
Add globe icon in the locale dropdown for better understanding #3121
Conversation
As pointed out in https://forum.securedrop.club/t/language-menu-is-confusing-when-the-user-does-not-read-the-current-language/407 language menu is confusing when the user does not read the current language. Adding a globe icon helps understand that the dropdown is about locales.
Codecov Report
@@ Coverage Diff @@
## develop #3121 +/- ##
========================================
Coverage 84.24% 84.24%
========================================
Files 34 34
Lines 2019 2019
Branches 219 219
========================================
Hits 1701 1701
Misses 262 262
Partials 56 56 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Check out my one comment inline
@@ -2,8 +2,9 @@ | |||
<div class="menu"> | |||
<input id="menu-1-checkbox" class="menu__checkbox visually-hidden" type="checkbox" > | |||
<label for="menu-1-checkbox" class="menu__trigger"> | |||
<span class="menu__trigger-arrow">▼</span> | |||
<i class="fa fa-globe menu__trigger-icon"></i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the source interface is intended to render well even with the Tor Browser security slider set to its safest setting. With the Tor Browser set to safest, unfortunately this icon does not render:
However converting the icon to a PNG should do the trick - that's what we did with the other icons on the source interface - see #1567 for more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Will do that. That answers my question of why some icons weren't visible in tor previously. Guess I fiddled with the safe settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, just curious, why isn't this needed in journalist interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question - so the recommendation for a source is to turn the security slider to highest to mitigate the threat of watering hole attacks, where Tor users are deanonymized using e.g. a JavaScript exploit. For journalists, the impact is not as severe since they don't have the same anonymity requirements as a source. That said, (as you know from your recent excellent CSS work on the Journalist Interface), we are trying to limit the amount of JavaScript on the source interface such that it is still functional if a journalist elects to turn up the security slider.
But since the security slider at the highest setting is not the recommended mode of operation on the Journalist Interface, it's OK if additional Font Awesome icons are added there (note there is a low priority issue in #1574 to replace the icons on the Journalist Interface with PNGs at some point).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@redshiftzero thanks for clearing that out. If we are reducing the amount of JS code in journalist interface then I think replacing icons with PNGs is also an important thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally, since you're making that PNG for the source interface anyway, if you elect to also use it on the journalist interface, it'd be a nice bonus to this PR 🎉 (but not required for merge)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I will use the globe PNG for both the interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@redshiftzero done. I have added the svg file as well just in case sometime in future we think of resizing the image or something, it will be easier from svg file. Let me know if I should remove the svg file from the PR though. 😃
Add both svg and png images for the globe icon to support safest security settings in tor browser in the source and journalist interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for the contribution @SaptakS
As pointed out here, language menu is confusing when the user does not read the current language. Adding a globe icon helps understand that the dropdown is about locales for people who don't read the current language.
Status
Ready for review
Description of Changes
Added globe icon before(left of) the locale name and moved the trigger arrow after(right of) the locale name.
Testing
How should the reviewer test this PR?
I did the following steps for testing:
SUPPORTED_LOCALES
list in thesecuredrop/config.py
file./i18n_tool.py translate-messages
./manage.py run
Checklist
If you made changes to the app code: