-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add [alt] attributes to images #9761
Add [alt] attributes to images #9761
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9761 +/- ##
==========================================
+ Coverage 16.06% 16.42% +0.36%
==========================================
Files 90 92 +2
Lines 4769 4907 +138
Branches 832 856 +24
==========================================
+ Hits 766 806 +40
- Misses 3480 3565 +85
- Partials 523 536 +13 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Souvik-Cyclic! I'm happy to merge this once the review notes have been addressed.
@@ -1,6 +1,6 @@ | |||
$def with (work, edition, page_url) | |||
|
|||
$def icon_link(link_class, img_src, link_text, login_redirect=True, ga_data=None): | |||
$def icon_link(link_class, img_src, link_text, login_redirect=True, ga_data=None, alt=""): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$def icon_link(link_class, img_src, link_text, login_redirect=True, ga_data=None, alt=""): | |
$def icon_link(link_class, img_src, link_text, login_redirect=True, ga_data=None): |
It isn't necessary to add alt
here, as it will be the same value for each modal link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing it from here is causing some issues on the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I see why this is happening. The alt
keyword argument needs to be removed from the icon_link
call on line 28. I overlooked that one during my review...
Let me test what you have now, and I can make a final commit to this branch that will remove the argument.
efd464f
to
56f98cd
Compare
@jimchamp review notes have been addressed, |
2dfb005
to
bf6ed2d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick updates! I noticed that the nav-arrow
changes affected that element's styling a good bit. Can you update the CSS for that, and push up the changes? Apologies for missing this earlier.
Edit: In order to test these changes, the .less
files need to be compiled to CSS. You can do this by running docker compose exec web make css
while the webapp is running.
6eb113b
to
018e86c
Compare
086bf24
to
2834dfc
Compare
for more information, see https://pre-commit.ci
Closes #9629
Adds [alt] attributes to images for Improving Accessibility Score of Book Pages
Screenshot
Stakeholders
@jimchamp