Update the extension to include form confirguration#555
Update the extension to include form confirguration#555dubdabasoduba wants to merge 1 commit intomasterfrom
Conversation
| * Created by keyman on 04/12/2018. | ||
| */ | ||
| public class JsonWizardFormActivity extends JsonFormActivity { | ||
| public class JsonWizardFormActivity extends FormConfigurationJsonFormActivity { |
There was a problem hiding this comment.
Why not add this to JsonFormBaseActivity or JsonFormActivity to support configurability of all kinds of forms and not just wizard forms?
There was a problem hiding this comment.
This extends the JsonFormActivity and JsonWizardFormActivity extends JsonFormActivity activity too. FormConfigurationJsonFormActivity can't extend JsonFormBaseActivity without a lot of refactor as it depends on a couple of functions on JsonFormActivity.
There was a problem hiding this comment.
Cool, but doesn't that still mean that form configurability with not be supported for JsonFormActivity?
There was a problem hiding this comment.
Yes, this upgrade doc explains why everyone should now extend FormConfigurationJsonFormActivity instead of JsonFormActivity
There was a problem hiding this comment.
Ok, I guess it's a bit asymmetric that we have JsonWizardFormActivity directly inheriting from FormConfigurationJsonFormActivity rather than creating a separate FormConfigurationJsonWizardFormActivity and asking people using wizard forms to migrate to that as well.
There was a problem hiding this comment.
It does make sense that FormConfigurationJsonFormActivity is a child of JsonFormActivity i.e. semantically it's a version of JsonFormActivity.
There was a problem hiding this comment.
On the other hand, it feels like all JsonFormActivitys should have the configurable feature available to them but not enabled by default. And so it would make more sense if it was added in the parent class and inherited by the child classes. Not sure if this indicates a use case for composition over inheritance here.
There was a problem hiding this comment.
Anyway, I don't see any functional issues with the approach. But I think it's worth pointing out the asymmetry. And noting that now we consider JsonWizardFormActivity to be a type of FormConfigurationJsonFormActivity and not directly a type of JsonFormActivity. Meaning that you will always have the configurability feature available (is it mandatory?) for wizards.
There was a problem hiding this comment.
Are the changes tested for compatibility?
2 participants
Fixes #554