Skip to content

Add support for new form type image-description #761

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tiritea
Copy link

@tiritea tiritea commented Mar 4, 2025

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, audio and video media types. It should not impact the existing functionality of these existing media types. XForm clients that do not support or recognize the new image-description form 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-image specified but without a corresponding image, because it is almost identical to the check now needed for image-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 new big-image check 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:

  • included test cases for core behavior and edge cases in tests
  • run python -m unittest and verified all tests pass
  • run ruff format pyxform tests and ruff check pyxform tests to lint code
  • verified that any code or assets from external sources are properly credited in comments - Not Applicable

Other important notes:

This is my first pyxform PR; please be gentle... :-)
Attn: @RuthShryock

…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.
Copy link
Contributor

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

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!

@lognaturel
Copy link
Contributor

You should be able to adapt/augment the big-image tests:

def test_big_image_invalid_if_no_image(self):
and throughout https://github.com/XLSForm/pyxform/blob/0d5dd3e8b10ac2cb57f25d8a5d6ad16250e3dad7/tests/test_translations.py

@tiritea
Copy link
Author

tiritea commented Mar 19, 2025

We've tried to document this..

Sorry, I'd completely missed that! Will do.

@lognaturel
Copy link
Contributor

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.

@tiritea
Copy link
Author

tiritea commented Apr 9, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants