Refactoring People Management UI to use Fragments #3970
Conversation
By mistake I've switched the layouts for the above files.
| super(context, attrs); | ||
| } | ||
|
|
||
| private ViewTreeObserver.OnPreDrawListener preDrawListener = null; |
There was a problem hiding this comment.
Same as above, this needs to follow the naming conventions. Cheers!
… to have it in Adapter
| PersonDetailFragment personDetailFragment = (PersonDetailFragment) fragmentManager.findFragmentByTag(KEY_PERSON_DETAIL_FRAGMENT); | ||
|
|
||
| long personID = person.getPersonID(); | ||
| int localTableBlogID = person.getLocalTableBlogId(); |
There was a problem hiding this comment.
Earlier in the file, a semantically same variable/parameter is named localBlogId. I'd suggest naming those two the same. I think that localTableBlogID is the more suitable name so, may I suggest renaming the parameter of the refreshUsersList() method above? Cheers!
|
Finished with my review pass @oguzkocer . I think the whole feature works rather well! After your amendments so far, the only notable comment I have is about the animations that leave something to be desired/tweaked. The rest of the comments are minor. Thanks! |
|
Thanks for the detailed review @hypest. I've fixed all the issues except the one about the animations and pinged more people to get some feedback on that. If there are no more issues, once we decide on that, we can get this merged in. |
|
@hypest I think I've fixed the animation issue after the config changes and explained the approach in the line comments. If you can test it and verify that's a good solution, we can merge this in once Travis does it's thing. Thanks! |
|
LGTM. 👍 |
Related #3901. This is the first time I am getting my hands dirty with Fragments. I've worked with them before, but never converted activities to fragments. I have to say that it turned out to be messier than I expected. Now that I know a few more things, if I need to do it again, it'll be easier, but it still doesn't feel clean.
Anyhow, after chatting with @hypest, we thought it's better to use fragments inside a single activity rather than different activities for People Management UI. This PR is an attempt to make that change. Basically
PeopleManagementActivityworks as an orchestrator and updates/refreshes the fragments when necessary. I've tried my best to follow the best practices here, so hopefully it's a good implementation.I have implemented a similar animation for the fragment slide in/out we have for activities, but there is still a bit of jankiness there I am not sure where it's coming from.
Please note that this is not the final version of the People Management UI, it just converts the previous version to Fragments and corrects a few mistakes I made in the earlier PRs. However, the only thing that's left should be just positioning & styling the current elements which will be handled in a separate PR.
/cc @astralbodies
Needs review: @hypest