-
Notifications
You must be signed in to change notification settings - Fork 15
Use substitution values in image alt and title text #1163
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
base: main
Are you sure you want to change the base?
Conversation
@@ -64,9 +70,10 @@ public class ImageBlock(DirectiveBlockParser parser, ParserContext context) | |||
public override void FinalizeAndValidate(ParserContext context) | |||
{ | |||
Label = Prop("label", "name"); | |||
Alt = Prop("alt"); | |||
Align = Prop("align"); | |||
Alt = (Prop("alt") ?? "{undefined}").ReplaceSubstitutions(context); |
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.
Could you please explain what this is doing?
Will this leave a {undefined}
in the alt property?
Generally, there is valid use case for an empty alt=""
property. See https://www.w3.org/WAI/tutorials/images/decorative/
Not sure if this still given with this condition.
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.
I stole borrowed from TabSetBlock.cs
🙃 but if an empty string is better, let's do that!
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.
Ah interesting. I also don't understand what it's doing in TabSet :D
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.
@Mpdreamz does this have a specific purpose?
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.
Uhh I can't think of any. Must have been a leftover from frenzy coding 😸
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.
I'm having some trouble figuring out the right syntax. Just changing "{undefined}"
to ""
is the only thing I could get to work, but I don't think it makes sense to replace subs in an empty string.... Any hints? 😏
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.
Maybe something like:
Prop("alt")?.ReplaceSubstitutions(context) ?? "";
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.
Ahhh the ?
after Prop("alt")
was the missing piece. Thanks!
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.
LGTM.
Only nit: Make it possible to set an empty alt
tag.
Fixes #814
This attempts to use substitution values in image
alt
andtitle
text. I've noticed some keys being flagged as "unused" when they are only used inalt
ortitle
text (example). I've also been building the site locally and using the HTML files in the.artifacts/assembly/
directory to look for content problems (including unresolved sub keys) and there were sooo many inalt
andtitle
text that it was a bit difficult to track content fixes.(I hope it's ok that I picked up this issue. During spacetime I'm trying to get more familiar with how docs-builder works so I can make tiny fixes like this one as needed in the future. 🪐 )