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 integration of images with surounding text using the figure element #196

Closed

Conversation

gonzalo-bulnes
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes commented Apr 10, 2021

Status

Work in progress

Description of Changes

Adds captions to all images in the documentation by using the figure directive. Each caption describes the image and hints that clicking on the image will display it in full size. Since the possibility of opening the image in full size is now explicit, this PR aslo slightly reduces the size of the images in the docs in order to reduce the interruption that illustrations cause to the flow of the text.

This is a follow up on the introduction of image captions in #141.

Pros

  • Image descriptions make content more accessible.
  • Images don't interrupt the text flow as much.
  • It is clear that images can be opened full-size.

Accepted trade-offs

  • Unlike the image directive, figure cannot be used in substitution definitions, which means that the image URLs cannot be grouped at the end of the document anymore. Because most images are used only once per document, I think that's an acceptable trade-off for the accessibility improvement.

Testing

I'll update when the PR is ready for review.

Release

No special considerations before release come to mind yet, I'll confirm when the PR is ready for review.

Example

image

Note: the pixellation on the image is due to the amount of zoom out I applied to keep the screenshot size small. The figures don't appear pixellated in the docs (example).

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
Copy link
Contributor Author

gonzalo-bulnes commented Apr 10, 2021

Touching base to check if this is desirable.

It's a quick run through the docs for me, so I think it's worth rolling out captions everywhere (and give a consistent pattern to follow for new docs). I'm also taking note of small UX inconsistencies as I compare the docs and the screenshots, so the read-through doesn't feel like wasted time.

That being said YMMV, hence the question before I go further 😉

@eloquence eloquence self-assigned this Apr 12, 2021
docs/admin.rst Outdated Show resolved Hide resolved
@eloquence
Copy link
Member

@gonzalo-bulnes Thank you for working on this! I'm broadly in favor of this change, but I think if we use captions throughout the docs like this, we may want to style them a little bit differently, e.g., a bit smaller and in dark grey font, so they don't interrupt the flow of the main body text.

I'm curious what other maintainers and contributors think about this change. An alternative approach would be to be much more selective, and only apply it to graphics where the detail really is important (like the overview diagram, and the detailed firewall config screenshots in https://docs.securedrop.org/en/stable/network_firewall.html).

@gonzalo-bulnes
Copy link
Contributor Author

I tend not to question the styles much when relying on a theme like Read The Docs'. I guess I tend to assume that readability being their core business, the themes would have reasonable defaults, but yours is a fair point. Getting people's thoughts seems like a good idea! 🙂

@gonzalo-bulnes
Copy link
Contributor Author

👋 As I was taking a look at the Netgate docs a few days ago, I noticed the way they use captions when including figures in their how-tos. Specifically, they use them to repeat the action you're supposed to perform when your see what the image show.
Today while reading your comments again, I just made the link and I wonder what you'd think @eloquence : )

Example in the Netgate docs: (source)

example-of-figure-captions-in-netgate-docs

@gonzalo-bulnes
Copy link
Contributor Author

Not directly related:

An alternative approach would be to be much more selective, and only apply it to graphics where the detail really is important [...]

When it comes to including "Click on the image to see it full-size", I agree it's probably not useful to mention it explicitly in all images. Besides, the feature doesn't depend on the caption itself.

@eloquence
Copy link
Member

eloquence commented Jun 18, 2021

@gonzalo-bulnes I like the Netgate example. The screenshots have arrow annotations, and the captions restate the specific step that users are asked to perform.

One thing I noticed is that your PR implicitly changes the ALT attribute for screenshots. For example, journalist-admin_interface_index.png currently has the moderately useful ALT attribute "SecureDrop admin home" in https://docs.securedrop.org/en/stable/admin.html

After your change, that ALT attribute is replaced with "_images/journalist-admin_add_user_totp.png". I believe this is because you are removing the image labels as part of transitioning to the figure syntax.

I would suggest the following design goals:

  • We should aim for ALT attributes that describe what the screenshot or image depicts. See discussion in Audit image labels for accessibility #70.
  • Captions should be included if/when they are useful to restate a specific instructional step, or to explain more complex images.
  • Navigational text like "Click to enlarge" should be styled to be unobtrusive or used selectively.

Do those goals make sense to you? If so, do you want to take another stab at a selective edit e.g. to the Admin Guide in line with these design goals?

@gonzalo-bulnes
Copy link
Contributor Author

Yes @eloquence , all three goals make complete sense to me! I'm happy to use the Admin Guide as a sample 👍

I hadn't noticed the change in the alt attributes, thank you for pointing it out. I'll have #70 in mind while I do this.

@gonzalo-bulnes
Copy link
Contributor Author

I kept the "Click to enlarge" hints when the screenshot displays large amount of text. It's admittedly an arbitrary threshold, and I'm not 100% sold on the idea.

On one side I wouldn't mind removing the hints entirely, on the other I think it's good to give people a change to learn that images can be zoomed - and that seems more relevant when there screenshot displays a large amount of information.
What do you think @eloquence?

@eloquence
Copy link
Member

Apologies for not responding sooner, @gonzalo-bulnes, thanks for the follow-up. I would not add the "Click to see full-size" hint to the larger SecureDrop screenshots; as you say, the size threshold is somewhat arbitrary, which could be confusing for documentation writers and readers alike.

I think the resize hint would be helpful in these cases:

The firewall screenshots in https://docs.securedrop.org/en/stable/network_firewall.html may also benefit from the hint. But I would err on the size of conservatism for now; perhaps we can come up with a more subtle affordance to make the resizability of images discoverable, separately from this PR.

@gonzalo-bulnes
Copy link
Contributor Author

Thanks @eloquence I'll apply that to see how it looks/reads 🙂

@eloquence
Copy link
Member

Hi @gonzalo-bulnes, just checking in - do you still have time to poke at this?

@gonzalo-bulnes
Copy link
Contributor Author

Hi @eloquence! The last couple of weeks were somewhat busier than I expected, and I postponed working on this two days ago. But yes, I intend to get to it over the weekend 🙂

@gonzalo-bulnes
Copy link
Contributor Author

(rebased, squashed commits and removed the hints)

@gonzalo-bulnes
Copy link
Contributor Author

gonzalo-bulnes commented Aug 30, 2021

Slow coming back into context.

Arguably, this PR is now more about using the figure element where useful, more than adding captions (the cases where captions are useful are sparse). Do you mind me renaming the PR @eloquence? I'm thinking about something in the lines of improving the images integration with the text, or specifically employing the figure element for improved readability.

With the focus on using the figure element, I could add the other pages to this same PR: those that could use captions (e.g. the diagrams you mentioned above) and where the flow of reading could use the narrower images. How does that sound to you?

@eloquence
Copy link
Member

Sounds great, please retitle as you see fit :)

@gonzalo-bulnes gonzalo-bulnes changed the title Add captions to images Improve integration of images with surounding text using the figure element Sep 1, 2021
@eloquence
Copy link
Member

@gonzalo-bulnes Just checking in on the status of this PR, would be great to get a first iteration over the finish line in the next couple of weeks :)

@gonzalo-bulnes
Copy link
Contributor Author

Hey, apologies for the slow reply, I plan on resuming this tomorrow morning : )

@eloquence
Copy link
Member

@gonzalo-bulnes If you're coming up from air sometime, this may be a nice slow-burn PR to return to :P

@eloquence
Copy link
Member

Hi @gonzalo-bulnes, this PR has been stale for a while, I'd suggest closing it out for now unless you want to return to it.

@eloquence eloquence closed this Jun 1, 2022
@eloquence eloquence reopened this Jun 1, 2022
@gonzalo-bulnes
Copy link
Contributor Author

I'm properly tracking this one now in my personal project board. 😬

@gonzalo-bulnes gonzalo-bulnes self-assigned this Aug 4, 2022
@eloquence
Copy link
Member

Hi @gonzalo-bulnes, I suggest we close this out or adopt it, but let me know if you have interest in picking it up again :)

@gonzalo-bulnes
Copy link
Contributor Author

I won't be resuming this in the near future, I'll close it an you can re-open whenever you see fit 🙂

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.

2 participants