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

Ui fixes #1538

Merged
merged 15 commits into from
Feb 5, 2017
Merged

Ui fixes #1538

merged 15 commits into from
Feb 5, 2017

Conversation

heartsucker
Copy link
Contributor

Makes some of the suggested changes in #1536 and fixes an incomplete rebase from #1349.

@heartsucker heartsucker force-pushed the ui-fixes branch 3 times, most recently from 64f0871 to 61f7365 Compare January 30, 2017 08:15
Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

Solid changes @heartsucker 👍 . I've added a couple of comments inline. Once addressed this is good to merge in.

font-size: 30px
color: #004080
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like from the mockups that the text that we want this color and left alignment applied to is actually the h2 text?

screen shot 2017-01-31 at 2 11 52 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the bigger problem is that most places we use h2, we should be using h1, and the only places we use h1, should definitely be h2. Ha. I'll make some more changes.

@@ -34,15 +34,16 @@
</h2>
<hr class="cut-out" />
<p>If this is your first time submitting documents to journalists, start here.</p>
<a href="/generate" class="btn alt block" id="submit-documents-button"><img id="warning-close" src="{{ url_for('static', filename='i/font-awesome/white/cloud-upload.svg') }}" width="17px" height="17px"> Submit Documents</a>
{# adding a break between 'submit' and 'documents' to make both containers equal sized #}
<a href="/generate" class="sd-button btn alt block" id="submit-documents-button">SUBMIT<br/>DOCUMENTS</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like these containers are not equal sized in Tor Browser:

screen shot 2017-01-31 at 1 46 07 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not seeing it display like this. What OS are you testing this on? I'm on Firefox 51.0.1 / Debian Stretch. Also, how are you getting TB to reach localhost?

Copy link
Contributor

@redshiftzero redshiftzero Feb 1, 2017

Choose a reason for hiding this comment

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

Check out this link for how to allow TB to reach localhost. I was running Tor Browser 6.5 on Mac OS X when I made that screenshot. Check it out in Tor Browser (it's based on Firefox 45.7.0 so the CSS might render differently) and let me know if you can't reproduce what I see above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "no proxy" option isn't working for me. Also TB 6.5. Hmm. If you want to branch this and make a tweak, go ahead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'll take a look

Copy link
Contributor

@redshiftzero redshiftzero Feb 2, 2017

Choose a reason for hiding this comment

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

Ok branched off this and in fix-box-sizes-on-src-interface-index there is a single commit 74f612b fixing these boxes to be the same size. Now in Tor Browser this looks like:

modified

I'll submit a PR against this repo to get that commit merged in after I merge your PR in


&:hover
color: #999
background-color: white
Copy link
Contributor

Choose a reason for hiding this comment

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

Verified this fixes the CSS on the journalist interface 👍

@redshiftzero
Copy link
Contributor

Hey @heartsucker - thanks for addressing these comments - everything looks good on my end now. If you fix the functional test failure I will merge this in 😄

@heartsucker
Copy link
Contributor Author

@redshiftzero Sounds good. It might take a few days for me to get to it. New job 'n all.

@heartsucker
Copy link
Contributor Author

Two things:

  1. How the eff do you get a container to be the width of the stuff inside it without setting min-width to some pixel value?

  2. Is cancel button outline / text supposed to be the same color as the continue button?

screenshot_2017-02-02_21-09-52

@redshiftzero
Copy link
Contributor

  1. Try width: auto - also if you're talking about for the buttons, if you remove the width: 35% from the btn primary then the button will be the width of the content inside it:

screen shot 2017-02-02 at 1 10 19 pm

  1. For that button we are using the color suggested by @ninavizz - @ninavizz is the different color intentional for a UX reason? Unless there is a UX reason to make them different, I think it's fine to change to be the same color (note you'll also need to modify the rollover color).

@ninavizz
Copy link
Member

ninavizz commented Feb 3, 2017

Hey @heartsucker—my bad on that hex call, sorry abt that! I got sloppy, it should visually match the primary button—yes, yes. :)

I updated my spec to cite #727c9b as the recco'd stroke color (it's a taaaad darker than the button blue).

Thx for catching & asking!

@ninavizz
Copy link
Member

ninavizz commented Feb 3, 2017

Ok, I created #1552 as a go-to place to solidly document global basic things between all three UIs: Journalist, Source, and Admin. I had used #004080 I had for hyperlink-blue, and confused that for the header's blue in my haste to document earlier notes. #7985AA is the global "muted blue" I want to see carry into headers. Really sorry for the runaround on that @heartsucker @redshiftzero @fowlslegs!

@heartsucker
Copy link
Contributor Author

if you remove the width: 35% from the btn

D'oh. Note to self: don't debug on the S-bahn. :(

@heartsucker
Copy link
Contributor Author

@redshiftzero Should be good to merge now.

@redshiftzero
Copy link
Contributor

@heartsucker I took another look at this - looking much better - just a couple more small things 🙏 :

There was a rebase snafu on /login:

screen shot 2017-02-03 at 10 06 09 am

  1. Remove extra codename field (leaving the one with id="login-with-existing-codename" since the functional tests are looking for that)
  2. Cancel button should be at the same y-position as the continue button (once you fix the nesting of the <p>s that should happen naturally 😇
  3. Cancel button should redirect back to the index

Finally, the index looks like this in Tor Browser:

screen shot 2017-02-03 at 10 05 52 am

  1. But my SecureDrop-CSS-spider-sense tells me that if you change the classes on the "Check for a response" button to sd-button btn primary index then this button will be fixed 💯

Also thanks @ninavizz for that quick response 💎

@ninavizz
Copy link
Member

ninavizz commented Feb 3, 2017

Are Sources now being asked to enter their passphrase 2x? :(

Cancel (or secondary) button should always be visually paired w/ the primary button—no more than ~4px space, between them. Once I'm fully competent with this Terminal/dev-build stuff (that @fowlslegs was extra-awesome spending THREE hours walking me through, on Tuesday!) that's still super brain-hurts-new to me, I'll do all I can to comment directly on the code itself.

Longer-term, a UI library of common elements—such as button pairs!—with "how to modify w/o breaking code or visual standards" notes will be a nice thing to create, so all these repeat-elements can be way easier to wham-bam cut-and-paste/modify. More time spent building + designing, less time spent replicating the saaaame things over and over again. The Y!UI library was built when I was at Yahoo!, and maan, that thing was awesome!

@heartsucker
Copy link
Contributor Author

heartsucker commented Feb 3, 2017 via email

@ninavizz
Copy link
Member

ninavizz commented Feb 3, 2017

Cool! I (and lots of others in the UX-for-security community) just find that whole "Enter your password in a second time!" pattern-standard to be absurdly asinine... and wanted to make sure it wasn't sought, here. I should trust my experience with you thus far, that you're not an advocate for piffy security-theatrics... :D

@heartsucker
Copy link
Contributor Author

@redshiftzero Apologies for the sloppy rebase and time wasted. This one should do it, though. However, two notes about things that look less than ideal. Not sure if these should be tackled here or in another issue.

  1. Without JS enabled, it is not obvious what the download and delete buttons do (IMO).

screenshot_2017-02-04_09-46-13

  1. The reset buttons here have too much text in them, so I think there should be text to the left explaining what the button does, and then reset totp and reset yubikey buttons.

screenshot_2017-02-04_09-46-50

@ninavizz Comments / suggestions?

@heartsucker heartsucker force-pushed the ui-fixes branch 2 times, most recently from 26d527d to 8729ece Compare February 4, 2017 18:37
@redshiftzero
Copy link
Contributor

@heartsucker no worries. Thanks for making these changes! I've checked this out and it now looks great - I'm going to merge this in now given that the changes are solid and we've already had a long back and forth in this PR. But! If you want to make another followup PR you could make the following changes to address these (IMHO valid but much smaller) concerns:

  1. Change button text to "Download Selected" and "Delete Selected" - which may involve making some CSS changes such that this text fits in one line when JS is enabled.

  2. I agree with your suggested solution here, bearing in mind that "Reset Google Authenticator" makes more sense to non-technical folk than "Reset TOTP" (people acting as administrators / point people for their SecureDrop instance might not be super techy).

@redshiftzero redshiftzero merged commit f54c531 into freedomofpress:develop Feb 5, 2017
@heartsucker heartsucker deleted the ui-fixes branch February 5, 2017 18:42
@heartsucker
Copy link
Contributor Author

@redshiftzero You got it. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants