People management v1 wrap-up #4086
Conversation
| if (!isAdded()) return; | ||
|
|
||
| Person person = PeopleTable.getPerson(mPersonID, mLocalTableBlogID); | ||
| Person person = getCurrentPerson(); |
There was a problem hiding this comment.
Nitpicking: Let's lose the "Current" from the function name as it has some connotations (like, a globally selected "current" user) beyond the actual usage here. What do you think about "loadPerson()"? Cheers!
Well, I should have added this comment on the function declaration line...
|
@oguzkocer , thanks for the effort on this one! I like how the feature operates. I've finished my review pass at this point. The only issue left, from my point of view, is the dismissal and re-opening of the role popup that doesn't always show the currently active role. Lemme know if I should provide more info on that and whether I should make another review pass. Cheers! |
| } | ||
|
|
||
| @SuppressWarnings("deprecation") | ||
| // Remove the selectableItemBackground if the user can't be edited |
There was a problem hiding this comment.
May I suggest reinstating this comment back to the line the function is called (line 177 at this point)? The function name is pretty clear and perfectly corresponding to its body, whereas the "if the user can't be edited" part is not implemented here but rather, at the caller's position. Sorry for the mind-blowing nitpicking! 😃 ❤️
I don't think this is a big deal in the sense that we have a lot of similar issues in the app. Ideally I agree that it shouldn't take that much time, but with the current architecture and this being a very edge case, most likely never even going to come up, I think it's fine to leave it as is. I'll still add this in the wrap-up post and give a heads up to Happiness Engineers. Thanks! |
|
@hypest I think I've addressed all the comments & issues so far. If you like the fixes for the issues you've raised I think the only thing left is figuring out the design issue you've pinged @rickybanister about. Thanks! |
| Bundle args = getArguments(); | ||
| if (args != null) { | ||
| String role = args.getString(ROLE_TAG); | ||
| mRoleListAdapter.setSelectedRole(role); |
There was a problem hiding this comment.
Let's also cover the case where the device is rotating with the dialog open. In the usual drill, we can use the savedInstanceState to pass the currently selected Role. Cheers!
|
@oguzkocer I finished my current pass. The only "issue" now is the rotation of the role dialog not keeping the selection and a quite minor thing with the "Administrator" text. Besides those, the design issue you mentioned (same styling of the person name and role fields) is not a blocker from my point of view. We can raise it as an issue and continue the discussion there. |
|
An alternative design would be to use the header in a bigger way like this:
|
| public void onSaveInstanceState(Bundle outState) { | ||
| super.onSaveInstanceState(outState); | ||
| String role = mRoleListAdapter.getSelectedRole(); | ||
| if (role != null) { |
There was a problem hiding this comment.
Being extra careful doesn't hurt but there is actually no need to check for null at this point. For simplicity's sake, I'd recommend removing the check as there is no semantic value in it.
I would even go one step further and say that you can also remove the null-check around line 136 (radioButton.setChecked(mSelectedRole.equalsIgnoreCase(role)) by reversing the equals to role.equalsIgnoreCase(mSelectedRole) as the role is never expected to be null. Cheers!
There was a problem hiding this comment.
You're right about the line 136 it makes it much better to do it the way you've suggested. However for this line, role could actually be empty if it wasn't passed during initialization and the user hasn't selected anything yet. So, I think it's good to make sure this is not null before writing into the bundle.
|
Thanks @rickybanister , that alternative design looks interesting and I can see it solving the styling issue I brought up! I'd most certainly want to see it in action as well! As mentioned earlier, I will not block this PR over this particular issue but I'll leave it to @oguzkocer to decide whether to give it a stab as part of this PR or defer it to a subsequent one. Cheers! |
|
I think it would be fine to merge this pr and work on that design in a new one. |
…ook" This reverts commit 29182ed.
Relevant #3901. I've started this PR with the intention of adding the change role & remove user functionality's UI side, but I kept finding small issues that I didn't feel deserved a separate PR. However, it got a bit out of hand and became a bigger PR than I'd have liked. Having said that, this should be the last PR, aside from adding Analytics, and we can wrap up the v1 of this feature once this is merged.
A list of things that was accomplished in this PR:
I've already added a bunch of comments to explain what certain parts of the code do, but if there is anything not immediately clear, please let me know so I can add more comments.
To test: Since this is the last PR, we should test the whole flow
Needs review: @hypest