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.

Sign up for GitHub

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

Jump to bottom

Mitigate absence of images in the User Guides #237

Merged
merged 3 commits into from
Jul 8, 2021
Merged

Mitigate absence of images in the User Guides #237

merged 3 commits into from
Jul 8, 2021

Conversation

gonzalo-bulnes
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes commented Jun 27, 2021 •
edited
Loading

Status

Ready for review

Description of Changes

Implements the approach proposed in #236 for the sections of the documentation that target end-users (sources, journalists, admins).

Instead of coming up with new ways of composing the text with the images, I've re-applied as much as possible the patterns that I've found achieved the purpose in other parts of the documentation.

Fixes Contributes to fixing #236
Mitigates the impact of #234
Contributes to fixing #70

Compatible with (but does not depend on, nor require) #196

Testing

  • Proof-read the paragraphs around each modification to ensure continuous reading of text and images makes sense.
  • Do the same without displaying the images (e.g. delete the images locally).

Release

No special considerations come to mind for release.

Checklist (Optional)

  • Doc linting (make docs-lint) passed locally
  • Doc link linting (make docs-linkcheck) passed
  • You have previewed (make docs) docs at http://localhost:8000

@gonzalo-bulnes gonzalo-bulnes force-pushed the mitigate-absence-of-images branch 2 times, most recently from 5f2f472 to a6cd27a Compare June 27, 2021 23:01
@gonzalo-bulnes
Copy link
Contributor Author

Updating the Install SecureDrop is less straightforward, so I'll limit this PR to the User Guides to make review easier.

@gonzalo-bulnes gonzalo-bulnes force-pushed the mitigate-absence-of-images branch from a6cd27a to 689a962 Compare June 28, 2021 12:37
@gonzalo-bulnes gonzalo-bulnes changed the title Mitigate absence of images Mitigate absence of images in the User Guides Jun 28, 2021
@gonzalo-bulnes gonzalo-bulnes marked this pull request as ready for review June 28, 2021 12:39
eloquence
eloquence reviewed Jul 2, 2021
View reviewed changes
docs/journalist.rst Outdated
Click **Save file**. In the save dialog, select one
of the two folders highlighted in red in the screenshot below:
Click **Save file**. In the save dialog, select one of the two folders
called **Tor Browser** and **Tor Browser (persitent)**.
Copy link
Member

Choose a reason for hiding this comment

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

typo: persitent->persistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

eloquence
eloquence reviewed Jul 2, 2021
View reviewed changes
docs/journalist.rst Outdated
@@ -594,7 +594,7 @@ Decrypting and Preparing to Publish
.. note::

To decrypt a VeraCrypt drive on a Windows or Mac workstation, you need
to have the VeraCrypt software installed. If you are unsure if you have the
to have the **VeraCrypt** software installed. If you are unsure if you have the
Copy link
Member

Choose a reason for hiding this comment

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

The bolding here seems a bit excessive, I think unless we are explicitly directing folks to a menu or part of the UI as part of referencing the name of the application, we can omit it (applicable to other additions of bolding for software names as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I'll review the other instances too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

eloquence
eloquence reviewed Jul 2, 2021
View reviewed changes
docs/source.rst Outdated
.. |Security Slider| image:: images/manual/source-turn-slider-to-high.png
:alt: Advanced Security Settings in Tor Borwser.
Copy link
Member

Choose a reason for hiding this comment

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

typo: Borwser->Browser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

eloquence
eloquence reviewed Jul 2, 2021
View reviewed changes
docs/admin.rst Outdated
@@ -823,12 +800,20 @@ left off. However, if the same issue persists, you will need to investigate
further.

.. |Reset Passphrase| image:: images/manual/screenshots/journalist-edit_account_user.png
:alt: The account edition form allows change name, reset passphrase, and reset two-factor authentication.
Copy link
Member

Choose a reason for hiding this comment

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

  • edition form->editing form
  • allows change name->allows admins to change name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

eloquence
eloquence requested changes Jul 2, 2021
View reviewed changes
Copy link
Member

@eloquence eloquence left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work on this, @gonzalo-bulnes. I really like the general accessibility principle you are applying here of making sure that the text speaks for itself, without relying on images the user may not be able to see.

The addition of descriptive ALT text is a huge improvement, as well, and switching to numbered lists instead of bullet lists for step-by-step instructions makes a lot of sense.

I noted a few small issues, and will need to take another careful read through since it's a fairly large diff, but I'm optimistic we can land this next week if you have time to work on it.

I suggest you rebase as well, to pick up the removal of LFS. I rebased locally and it seemed to apply without issue.

@gonzalo-bulnes
Copy link
Contributor Author

Thank you for you review @eloquence! I clearly missed a spellcheck run, sorry about that. I've got time tomorrow to address your comments and rebase the branch, I'll do that 🙂

@gonzalo-bulnes gonzalo-bulnes force-pushed the mitigate-absence-of-images branch from 689a962 to c4e1024 Compare July 3, 2021 13:48
@gonzalo-bulnes
Copy link
Contributor Author

I applied the suggestions, performed a general spelling check and rebased.

@gonzalo-bulnes gonzalo-bulnes requested a review from eloquence July 3, 2021 13:54
@gonzalo-bulnes
Consistently italicize application names...
be2571e 
...but reserve bold for specific user interface elements.
@gonzalo-bulnes gonzalo-bulnes force-pushed the mitigate-absence-of-images branch from c4e1024 to be2571e Compare July 3, 2021 13:56
eloquence
eloquence approved these changes Jul 8, 2021
View reviewed changes
Copy link
Member

@eloquence eloquence left a comment

Choose a reason for hiding this comment

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

Looks great, thanks again. (In case it's helpful for others: I used a bookmarklet for converting ALT attributes to TITLE attributes to preview the ALT attributes as tooltips.)

@eloquence eloquence merged commit e390a4d into freedomofpress:main Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eloquence eloquence eloquence approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gonzalo-bulnes @eloquence