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

Issue 2508: Replaces pancake icon with cloud upload, adds checkmark-i… #3535

Merged
merged 2 commits into from Nov 22, 2018

Conversation

Projects
None yet
3 participants
@kvinton
Copy link
Contributor

kvinton commented Jun 17, 2018

…n-a-circle and x-in-a-circle to submit and cancel

Status

EDIT: In progress. I'll address these tests failing and then send another message.

Description of Changes

Fixes #2508. Changes pancake icon to cloud upload and adds checkmark and x icons to "submit" and "cancel."

Checklist

I think I may be missing something in terms of what tests I'm supposed to be running for a pull request, so would appreciate feedback on that.

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to the system configuration:

@kvinton kvinton requested a review from Jun 17, 2018

@redshiftzero
Copy link
Member

redshiftzero left a comment

Thanks for the PR @kvinton, this looks good! I've dropped some notes/explanations inline for the SecureDrop quirks of relevance (😇) to your changes, take a peek.

In terms of which tests to run for a change of this type, the linting (make lint) and the application tests (make -C securedrop test) are the right ones

@@ -28,7 +28,7 @@ <h1 class="headline">{{ gettext('Submit Materials') }}</h1>
<input name="csrf_token" type="hidden" value="{{ csrf_token() }}">
<div class="snippet">
<div class="attachment grid-item center">
<img class="center" src="{{ url_for('static', filename='i/server_upload.png') }}" width="73px" height="62px">
<img class="center" src="{{ url_for('static', filename='i/font-awesome/cloud-upload-blue.png') }}" width="80px" height="56px">

This comment has been minimized.

@redshiftzero

redshiftzero Jun 18, 2018

Member

So this i/font-awesome/cloud-upload-blue.png image we have in the repo is very tiny, only 68 pixels x 47 pixels, so we'd want to add a higher resolution version to the repository so that it's not stretched, right now if you zoom it's a wee bit pixellated:

screen shot 2018-06-18 at 9 51 51 am

This comment has been minimized.

@kvinton

kvinton Jun 19, 2018

Contributor

Thanks for all the feedback, @redshiftzero! I'll switch this image out for a higher res one, likely from the site you linked me for getting PNGs as I only see a SVG version on font awesome's website (at least without a pro account).


<a href="{{ url_for('main.lookup') }}" class="btn secondary" id="cancel">
<i class="far fa-times-circle"></i> {{ gettext('CANCEL') }}
</a>

This comment has been minimized.

@redshiftzero

redshiftzero Jun 18, 2018

Member

I like this change, it feels intuitive to me:

screen shot 2018-06-18 at 9 55 47 am

For the SecureDrop source interface, we want the web application to function nicely in Tor Browser with the security slider set to its safest state (which we recommend for source security). The thing is that FontAwesome SVGs actually don't display with this setting:

screen shot 2018-06-18 at 10 13 18 am

That's the reason why we have PNGs for the other icons elsewhere on the source interface. So we'll want to replace these SVG icons with PNGs. I used to use this site to create icon PNGs of the appropriate color, but sadly it's down, but this site looks like it should do the trick. This is also why we have the off-hover and on-hover CSS classes: such that the image changes to the image with the appropriate color on hover. Btw, the functional tests here are actually looking for the off-hover and on-hover CSS classes so once that is added for these new icons, the functional tests should pass 🤘.

@redshiftzero

This comment has been minimized.

Copy link
Member

redshiftzero commented Jun 18, 2018

Ah and one other note: we use AppArmor to specifically whitelist the files that Apache is allowed to access on the SecureDrop application server, so any new image files should get added to the Apache AppArmor profile here. For example, if you wanted to add a new image file called securedrop/static/i/font-awesome/cloud-upload-blue-large.png (which we want Apache to be able to read only), you'd make the following change:

diff --git a/install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/files/usr.sbin.apache2 b/install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/files/usr.sbin.apache2
index 017d9eb7..9c91e078 100644
--- a/install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/files/usr.sbin.apache2
+++ b/install_files/ansible-base/roles/build-securedrop-app-code-deb-pkg/files/usr.sbin.apache2
@@ -241,6 +241,7 @@
   /var/www/securedrop/static/i/securedrop_small.png r,
   /var/www/securedrop/static/i/font-awesome/cloud-upload-blue.png r,
   /var/www/securedrop/static/i/font-awesome/cloud-upload-white.png r,
+  /var/www/securedrop/static/i/font-awesome/cloud-upload-blue-large.png r,
   /var/www/securedrop/static/i/font-awesome/comments-blue.png r,
   /var/www/securedrop/static/i/font-awesome/comments-white.png r,
   /var/www/securedrop/static/i/font-awesome/exclamation-triangle-black.png r,

and so on for any other new image files.

@redshiftzero

This comment has been minimized.

Copy link
Member

redshiftzero commented Jul 11, 2018

Hey @kvinton just bumpin' this, let me know if you have questions or want a hand for any of the above!

@ultimatecoder

This comment has been minimized.

Copy link
Contributor

ultimatecoder commented Oct 18, 2018

@redshiftzero Because @kvinton isn't responding, I would be interested in working on this PR. Thanks

@kvinton

This comment has been minimized.

Copy link
Contributor

kvinton commented Oct 19, 2018

Hi @redshiftzero, I'm really sorry for not responding earlier. I'm happy to either finish this up this weekend or pass it off to @ultimatecoder.

@redshiftzero

This comment has been minimized.

Copy link
Member

redshiftzero commented Oct 19, 2018

hey! no worries - feel free to take a peek this weekend if you get a chance but if you want to hand off to @ultimatecoder that's cool too :-)

@kvinton

This comment has been minimized.

Copy link
Contributor

kvinton commented Oct 19, 2018

Sounds good! I'll work on it this weekend :)

@ultimatecoder

This comment has been minimized.

Copy link
Contributor

ultimatecoder commented Oct 21, 2018

@kvinton No worries. Thanks :)

@kvinton kvinton requested review from conorsch and msheiny as code owners Oct 22, 2018

@kvinton kvinton force-pushed the kvinton:issue-2508 branch from 2fdb1c9 to 188a13d Oct 22, 2018

@kvinton

This comment has been minimized.

Copy link
Contributor

kvinton commented Oct 22, 2018

@redshiftzero Sorry if you got a bunch of notifications. Github is being extremely glitchy tonight, but I addressed your comments and will take a look at the tests again in the morning!

@ultimatecoder

This comment has been minimized.

Copy link
Contributor

ultimatecoder commented Nov 17, 2018

@kvinton Thanks for pushing the changes. I am yet to review them in detail, but I have below quick comments for you. Please write back if you have any confusions on them.

  1. CI is failing tests for your fix. Apologies for missing this initially when I commented. Will you please try to debug the reason for them?
  2. I can see, your branch has two commits. A fix/feature should be single isolated commit. I request to squash your 2 commits to a single commit.
  3. You should sign your commit. Please read this guide and sign your commits in future too!

@redshiftzero I request to add any points you think I am missing here. Thanks 🤓

Issue 2508: Replaces pancake icon with cloud upload, adds checkmark-i…
…n-a-circle and x-in-a-circle to submit and cancel

@redshiftzero redshiftzero force-pushed the kvinton:issue-2508 branch from 188a13d to d974ee0 Nov 21, 2018

@redshiftzero
Copy link
Member

redshiftzero left a comment

excellent, thanks for the contribution @kvinton! (and apologies for the delay in review)

It turns out the CI failures were only due to the base branch being a bit behind and a minor linting failure due to the use of the style attribute in the HTML template, so I updated the base branch and added a wee commit to resolve. Approved for merge once CI passes

@redshiftzero redshiftzero merged commit 1b5ea7d into freedomofpress:develop Nov 22, 2018

5 checks passed

ci/circleci: admin-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: staging-test-with-rebase Your tests passed on CircleCI!
Details
ci/circleci: tests Your tests passed on CircleCI!
Details
ci/circleci: updater-gui-tests Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment