-
Notifications
You must be signed in to change notification settings - Fork 137
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
Add support for big-image #635
Conversation
pyxform/survey_element.py
Outdated
# Guidance hint alone is not OK since they may be hidden by default. | ||
if not any((self.label, self.media, self.hint)) and self.guidance_hint: | ||
raise PyXFormError(msg) | ||
|
||
# big-image must combine with image | ||
if not ("image" in self.media) and "big-image" in self.media: |
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.
Suggest if "image" not in self.media and "big-image" in self.media
pyxform/survey_element.py
Outdated
# Guidance hint alone is not OK since they may be hidden by default. | ||
if not any((self.label, self.media, self.hint)) and self.guidance_hint: | ||
raise PyXFormError(msg) | ||
|
||
# big-image must combine with image | ||
if not ("image" in self.media) and "big-image" in self.media: | ||
raise PyXFormError("You must specify an image in order to use big-image.") |
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.
Suggest raise PyXFormError(f"In order to use big-image, you must specify an image for the survey element named {self.name}
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. 2 minor suggestions around validation/message.
One other thought, is it fine that the translation test |
Yes, this is fine and good! |
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. |
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, XLSForm/xlsform.github.io#234
Before submitting this PR, please make sure you have:
tests
nosetests
and verified all tests passblack pyxform tests
to format code