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

alt option for image() #559

Merged
merged 6 commits into from
Apr 13, 2021
Merged

Conversation

Alex-Jordan
Copy link
Contributor

Currently to get alt text in an HTML image, you must awkwardly use extra_html_tags. This adds alt as an option, which is not only less awkward, but also allows PreTeXt to do its natural thing with an alt text description.

@Alex-Jordan
Copy link
Contributor Author

Thanks, I didn't know I had access to something like that without adding a package. Added that, and remobed the ternary operator I was using, which felt too cluttered here.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

Looks good. I tested it and it works well.

@drgrice1
Copy link
Member

drgrice1 commented Apr 6, 2021

There is one more thing that should probably be done here. That is add this to the usage above in the PHbasicmacros.pl file so this gets into the POD documentation.

@Alex-Jordan
Copy link
Contributor Author

So I looked at that just now.

  • There is an example with an align attribute. That seems to be deprecated, so I will maybe change that example to put some inline style attribute, unless you have a better suggestion.
  • I did not realize you can pass an array reference to image with more than one image. So maybe alt should be able to accept an array reference as well. I'll look into that.

@drgrice1
Copy link
Member

drgrice1 commented Apr 7, 2021

Yes, the align attribute has been deprecated for a long time. Change that.

@Alex-Jordan
Copy link
Contributor Author

It seems to me that there are legacy subroutines images(), caption(), captions(), imageRow(), and that syntax image([$oneimage,$anotherimage],...) only exists to support these legacy things. Also, these legacy subroutines do show up in the OPL.

I propose removing the last three example lines from the POD. Do you see any reason to handle it differently?

@drgrice1
Copy link
Member

drgrice1 commented Apr 7, 2021

I noticed that the header on the part for the image sub is "Macros for displaying static images". Perhaps at one point only static images would work, but now dynamically generated images also work. That should probably be changed.

The POD in the PGbasicmacros.pl file really needs some help. It seems that not a lot of time was spent in creating what is there, as well as things have changed and the documentation was not updated. The quick usage and example statements could be greatly improved with a little explanation.

See https://webwork.maa.org/pod/pg/macros/PGbasicmacros.html#Macros-for-displaying-static-images for what this looks like with the WeBWorK 2.15 code.

I am not suggesting that you do this here. Just pointing this out. If POD documentation were improved, that would make the other "documentation" efforts easier.

@Alex-Jordan
Copy link
Contributor Author

Unless I forgot something, this is ready for another look. Summary:

  • POD editing
  • You can pass alt => 'alt text' or alt => ['alt text 1', 'alt text 2', ...]
  • If the image is just an image (not an array ref) then either the alt string will be used, or the first entry if alt is an array ref
  • If the image is an array ref and the alt is just a string, the alt string will be repeated so many times
  • If the image is an array ref and the alt is also an array ref, the entries from the alt array ref will be used until they are used up. Any additional images get no alt text.
  • Now in scalar context, if image() was passed an array ref of images, it returns the join of their strings with a space in between. Formerly it only returned the first one. It's possible somewhere, something intentionally relied on that behavior. But it seems unlikely. And the OPL has no instances of image([ so I suspect that image() was never used that way. It gets passed an array ref by some deprecated legacy subroutines, and those ones use it in array context.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

This looks good.

Note:  Don't use comments in POD.  POD is not Perl code.
@drgrice1
Copy link
Member

@Alex-Jordan: I added a pull request to this pull request with some suggested changes to the POD documentation. There is no change to the wording, just the formatting of the generated POD output.

Improve the generated POD output from PGbasicmacros.pl.
@drgrice1
Copy link
Member

I think this is good to go. I will go ahead and merge it so you can continue work on your related pull request.

@drgrice1 drgrice1 merged commit b0d5af8 into openwebwork:PG-2.16 Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants