Skip to content
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.

Sign up for GitHub

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

Update the extension to include form confirguration #555

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

Update the extension to include form confirguration #555

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...son-form-wizard/src/main/java/com/vijay/jsonwizard/activities/JsonWizardFormActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
/**
* Created by keyman on 04/12/2018.
*/
public class JsonWizardFormActivity extends JsonFormActivity {
public class JsonWizardFormActivity extends FormConfigurationJsonFormActivity {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add this to JsonFormBaseActivity or JsonFormActivity to support configurability of all kinds of forms and not just wizard forms?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, but doesn't that still mean that form configurability with not be supported for JsonFormActivity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this upgrade doc explains why everyone should now extend FormConfigurationJsonFormActivity instead of JsonFormActivity

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does make sense that FormConfigurationJsonFormActivity is a child of JsonFormActivity i.e. semantically it's a version of JsonFormActivity.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes tested for compatibility?


@Override
public void initializeFormFragment() {
Expand Down