docs: minor improvements and fixes #1022
Conversation
github-actions
bot
added
the
documentation
📊 Package size report -0.03%↓
🤖 This report was automatically generated by pkg-size-action |
📊 Package size report -0.03%↓
🤖 This report was automatically generated by pkg-size-action |
There was a problem hiding this comment.
Left a few comments (mostly fairly minor) around:
- management of the new plugin dependency (not being used in the CI/CD build)
- accessibility (add
altattribute to images)
The only mild issue from my side is that at least on mobile this specific lightbox seems to make things worse. What I mean by that is that mobile users can simply pan & zoom natively and the result is actually better (see video below):
RPReplay_Final1658224864.mp4
In the first half I just pan & zoom with my finger and I am able to see the image at full-res while instead when I open the lightbox (which I still need to zoom) I am shown a thumbnail that has low res.
Looking at the settings of this plugin there seems not to be a straightforward way to disable it on mobile only.
| @@ -1,2 +1,2 @@ | |||
| FROM squidfunk/mkdocs-material | |||
| RUN pip install mkdocs-git-revision-date-plugin | |||
| RUN pip install mkdocs-git-revision-date-plugin mkdocs-glightbox | |||
There was a problem hiding this comment.
What about doing RUN pip install -r requirements.txt and add the dependencies there?
During the documentation build & publish process we actually read that file, so adding it only here means we'll have the new dependency only during development.
If we centralize everything in the requirements.txt file we can pin the version as well as avoiding this kind of oversight again.
There was a problem hiding this comment.
I agree with you!
There was a problem hiding this comment.
So we install utilities in 2 ways:
- Local env: via docker, and the squidfunk/mkdocs-material image already contains some dependencies
- Github Actions: via pip commands, installing dependencies one by one in the containers of the CI/CD pipeline
So in this case it would not make sense to reuse the same requirements.txt file as some of them would be redundant.
docs/core/logger.md
Outdated
| <br /> | ||
|
|
||
| <figure> | ||
| <img src="../../media/logger_utility_showcase.png" loading="lazy" /> |
There was a problem hiding this comment.
Can we also add a alt attribute to each image?
The alt attribute holds a text description of the image, which isn't mandatory but is incredibly useful for accessibility — screen readers read this description out to their users so they know what the image means. Alt text is also displayed on the page if the image can't be loaded for some reason: for example, network errors, content blocking, or linkrot.
There was a problem hiding this comment.
Yes! Absolutely. Well spotted.
There was a problem hiding this comment.
Let me know if they look good enough!
mkdocs.yml
Outdated
| effect: zoom | ||
| width: 100% |
There was a problem hiding this comment.
minor: this might be redundant. According to the docs these are all default values. If this was a conscious decision in order to explicitly show the settings then you can ignore this comment.
📊 Package size report -0.03%↓
🤖 This report was automatically generated by pkg-size-action |
* docs: fix formatting issues in readme * docs: formatting fixes and improvements * docs: metrics image caption fix * docs: readme/index bottom sections fixes * docs: changed order credits * docs: added CI/CD pip requirments + alt for images
Description of your changes
Follow-up PR from #1021.
Changes introduced:
How to verify this change
Docs:
npm run docs-website-build-runRelated issues, RFCs
N/A
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.