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.

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

Feat fragment form #27

Merged
merged 2 commits into from
Sep 28, 2016
Merged

Feat fragment form #27

merged 2 commits into from
Sep 28, 2016

Conversation

aveuiller
Copy link
Contributor

Hello,

This pull request mainly adds the class FormFragment, a pre-built Fragment embedding a Form.

I tried to factorize the Form initialiazation code within a new class: FormManager.
This new class is responsible for creating a FormController and managing the retained FormModel,
since this part was the same in FormActivity, FormWithAppCompatActivity, and FormFragment.

With this new architecture, the user only has to provide a FormCreation implementation (containing the initForm(FormController) method)
to the FormManager which greatly simplify the form embedding.

What do you think about it?

Copy link
Owner

@dkharrat dkharrat left a comment

Choose a reason for hiding this comment

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

I appreciate your contributions! I have minor comments, but otherwise it looks great!

formController.setModel(retainedModel);
recreateViews();
formManager = new FormManager(this, this, R.id.form_elements_container);
formManager.initialize();
Copy link
Owner

Choose a reason for hiding this comment

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

Given that the initialize() call is happening after the construction, it seems like it would be better to just call it directly from the constructor in FormManager. That way, the caller does not have to worry about manually calling .initialize().

/**
* Provides method inflating the form initial content.
*/
public interface FormCreation {
Copy link
Owner

Choose a reason for hiding this comment

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

I believe FormInitializer would be a good name for this interface.

Centralize Form lifecycle in a FormManager class,
which greatly simplify creation of a Form in Activities and Fragments
@aveuiller
Copy link
Contributor Author

I made the changes according to your comments.

@dkharrat dkharrat merged commit 31437f9 into dkharrat:master Sep 28, 2016
@aveuiller aveuiller deleted the feat-fragmentForm branch September 29, 2016 07:42
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.

None yet

2 participants