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

Improve Source Interface /lookup UX #5096

Merged
merged 2 commits into from Jan 28, 2020

Conversation

DrGFreeman
Copy link
Contributor

@DrGFreeman DrGFreeman commented Jan 15, 2020

Status

Ready for review.

Description of Changes

Fixes #5080.

Changes proposed in this pull request:

Implement source interface UX changes on /lookup as per #5080:

  • Move codename hint section to top of page.
  • Adjust codename hint section width and make responsive.
  • Increase vertical space between codename and "Submit" h2.
  • Set empty replies area backgroud to light grey.
  • Increase height of empty replies area.
  • Change empty replies text to "- No Messages -".
  • Change empty replies text p to h4.
  • Change empty replies text color to grey.

Testing

With the exception of reordering and moving element in the templates, there are not changes to the application logic. No changes in behaviour are therefore expected. The following tests are nevertheless recommended:

Codename hint position and visibility on subsequent logins

  1. Access /lookup as a new source.
  2. Check the codename hint is visible at the top of the panel, left of the "LOGOUT" button as per the screenshots below.
  3. Check that the "Show" / "Hide" links in the codename hint work as expected.
  4. Submit a document or message.
  5. Check that the "Forgot your codename?" link in the flash message makes the codename hint visible.
  6. Logout.
  7. Login as the same source.
  8. Check that the codename hint is not visible any more.

Read Replies section

  1. Access /lookup as a source.
  2. Check the "- No Messages -" item displays as per the screenshot below.
  3. Logout.
  4. Login to the Journalist Interface.
  5. Reply to the source.
  6. Log back in as the same source as in 1.
  7. Check that the reply displays as expected in the "Read Replies" section.
  8. Delete the reply.
  9. Check that the "- No Messages -" item displays as per the screenshot below.

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

N/A.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container
    N/A.

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR
    N/A.

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally
    N/A.

If you added or updated a code dependency:

Choose one of the following:

  • I have performed a diff review and pasted the contents to the packaging wiki
  • I would like someone else to do the diff review
    N/A.

Implement source interface UX changes on /lookup as per freedomofpress#5080:
- Move codename hint section to top of page.
- Adjust codename hint section width and make responsive.
- Increase vertical space between codename and "Submit" H2.
- Set empty replies area backgroud to light grey.
- Increase height of empty replies area.
- Change empty replies text to "- No Messages -".
- Change empty replies text p to h4.
- Change empty replies text color to grey.
@DrGFreeman
Copy link
Contributor Author

DrGFreeman commented Jan 15, 2020

Screenshots

Full page, default Tor Browser width (1000px)
image

Codename visible, width = 1000px
image

width > 880px (cross-over point to full width)
image

Codename visible, width > 880px (cross-over point to full width)
image

width <= 880px (cross-over point to full width)
image

Comments

  1. I added a small margin on top of the codename hint so that it is vertically centered with the logout button (as it seems to be in the right mockup from @ninavizz). When the codename is visible, it looks a bit off as visible in screenshot above. I could remove the margin so it aligns with the top of the logout button.
  2. I used the colors defined in securedrop/sass/_variables.sass. These do not exactly match those in the mockup. I can use custom colors to match the mockup exactly.

@eloquence eloquence added this to Ready for Review in SecureDrop Team Board Jan 17, 2020
@ninavizz
Copy link
Member

This looks great Dr G, thank you!

  1. I like the margin—it should look "less broken" when closed (most of the time), than when open (infrequently).
  2. Thanks for the nudge on the mockup colors not matching. As with the above, your gut was correct and I appreciate you using the defined colors in the SASS.

I'm happy to give this my thumbs-up, for the issue as spec'd.

@DrGFreeman
Copy link
Contributor Author

@ninavizz, Thanks for the feedback!

Knowing you're satisfied with the layout, I'll do some more manual testing to ensure I haven't introduced any unwanted behavior (lesson learned from previous PRs) and switch the PR to "Ready for Review" when done.

@DrGFreeman
Copy link
Contributor Author

@eloquence, @zenmonkeykstop, FYI, I changed the PR status from Work in progress to Ready for review and filled testing & checklist. This is ready for your review.

@zenmonkeykstop zenmonkeykstop self-assigned this Jan 22, 2020
@zenmonkeykstop
Copy link
Contributor

@DrGFreeman just FYI, I got into the weeds on test docs for the Qubes-based workstation today, but will be taking a spin through this PR tomorrow at the latest. Apologies for the delay!

Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Ran through test plan as follows:

Codename hint position and visibility on subsequent logins

  • Access /lookup as a new source.
  • Check the codename hint is visible at the top of the panel, left of the "LOGOUT" button as per the screenshots below.
  • Check that the "Show" / "Hide" links in the codename hint work as expected.
  • Submit a document or message.
  • Check that the "Forgot your codename?" link in the flash message makes the codename hint visible.
  • Logout.
  • Login as the same source.
  • Check that the codename hint is not visible any more.

Read Replies section

  • Access /lookup as a source.
  • Check the "- No Messages -" item displays as per the screenshot below.
  • Logout.
  • Login to the Journalist Interface.
  • Reply to the source.
  • Log back in as the same source as in 1.
  • Check that the reply displays as expected in the "Read Replies" section.
  • Delete the reply.
  • Check that the "- No Messages -" item displays as per the screenshot below.

Also did a diff review. One concern was the logout button being suppressed on pages other than /lookup (see comment)

@@ -30,10 +30,6 @@
{% endblock %}

<div class="panel selected">
{% if 'logged_in' in session %}
Copy link
Contributor

Choose a reason for hiding this comment

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

The logout button was moved from the base to lookup template as part of the layout changes. One consequence of this is that it no longer appears on other pages that use extend base.html - /why-journalist-key and the error pages being the most likely examples. It might be better to have the logout widget as an include, and include it on pages that may be rendered during an active session.

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 actually didn't need to move the logout button, only ensure the HR that follows the button in base.html is not included when the codename hint is displayed in /lookup. The changes in 08edfa2 restores the button while keeping the /lookup layout as desired.

- Restore logout button in base.html so it displays on other pages.
- Add conditional in template to prevent HR to display when codename
hint is displayed.
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

LGTM with restoration of LOGOUT!

@zenmonkeykstop zenmonkeykstop merged commit e1c80cd into freedomofpress:develop Jan 28, 2020
SecureDrop Team Board automation moved this from Ready for Review to Done Jan 28, 2020
@DrGFreeman
Copy link
Contributor Author

LGTM with restoration of LOGOUT!

Great! Thanks @zenmonkeykstop. Sorry you had to review twice.

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

Successfully merging this pull request may close these issues.

UX follow-up to multiple-codename generation case
3 participants