XLSForm / pyxform

A Python package to create XForms for ODK Collect.
BSD 2-Clause "Simplified" License
77 stars 134 forks source link

Add support for big-image #635

Closed lognaturel closed 1 year ago

lognaturel commented 1 year ago

Closes #34

Why is this the best possible solution? Were any other approaches considered?

I tried to follow the implementation for other media types exactly. I didn't consider any alternatives.

I considered not adding big-image to all of the translation tests but I decided it would feel more consistent to do so.

What are the regression risks?

This is an additive change that doesn't touch much existing logic so regression risks are low. There's some possibility of regression with other media types but they're well tested.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Yes, https://github.com/XLSForm/xlsform.github.io/issues/234

Before submitting this PR, please make sure you have:

lindsay-stevens commented 1 year ago

One other thought, is it fine that the translation test test_missing_translation__one_lang_all_cols__warn__default passes, seeing as the question type is note and there's a big-image specified?

lognaturel commented 1 year ago

the question type is note and there's a big-image specified

Yes, this is fine and good! big-image can be used to provide a bigger image either for a question label or for a select choice. A note is a read-only text question and can have the same label types as other question types.

lognaturel commented 1 year ago

For posterity/anyone watching who may have opinions, I want to acknowledge that this is the first column with a - in its name, all others use underscore.

I went with it because it matches the underlying XForm spec and is more of a type/variant than the other column headers.