Add support for new form type image-description#761
Add support for new form type image-description#761tiritea wants to merge 1 commit intoXLSForm:masterfrom
Conversation
…ranslatable string description for survey and choice option images. Add associated tests. Add additional error test for 'big-image' without a corresponding 'image' for choice options.
lognaturel
left a comment
There was a problem hiding this comment.
Thanks for this! The implementation looks good to me but the tests should not introduce real XLSX files or xml files 😬 We've tried to document this at https://github.com/xlsform/pyxform?tab=readme-ov-file#writing-tests and all new tests follow that approach. Hopefully it will be a relatively quick conversion to that format.
@lindsay-stevens please let me know whether you'd like to have a look too or whether you're ok with my review. Thanks!
|
You should be able to adapt/augment the big-image tests: pyxform/tests/test_sheet_columns.py Line 69 in 0d5dd3e |
Sorry, I'd completely missed that! Will do. |
|
We just merged #746 which causes some conflicts. If you can address them, that'd be great. But I can also take over the PR once you've updated the tests and do that part. |
|
It looks one of the conflicts is due to the parallel change that happened regarding you specify aliases and survey headers. So the new 'image-description' now just needs to be reformatted to match. |
Closes ##759
Closes ##760
Why is this the best possible solution? Were any other approaches considered?
This solution and others considered were discussed extensively on the ODK Forum. See https://forum.getodk.org/t/placeholder-alt-text-for-images-eg-screen-readers/50868
What are the regression risks?
This adds new new (optional) translatable form type, to the existing
image,big-image,audioandvideomedia types. It should not impact the existing functionality of these existing media types. XForm clients that do not support or recognize the newimage-descriptionform type can safely ignore it with no impact on existing XForm functionality.This PR also adds a new testcase for a choice option with a
big-imagespecified but without a correspondingimage, because it is almost identical to the check now needed forimage-description. This error condition is currently not being checked, and results in an invalid XForm being generated [note, this image prerequisite is being checked in the main survey, but not for choice options. It was probably overlooked]. The newbig-imagecheck will mean previously invalid XLSForms with this inconsistency will now fail to be processed, but I believe this is actually the desired behavior.Does this change require updates to documentation? If so, please file an issue here and include the link below.
Yes, this new ODK form type will need to be documented in the XLSForm Docs.
See XLSForm/xlsform.github.io#279
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint codeOther important notes:
This is my first pyxform PR; please be gentle... :-)
Attn: @RuthShryock