-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
"Repeat group picker" UI for FormHierarchy #2740
Conversation
0510030
to
b3aa5dc
Compare
This doesn't yet render anything new.
This ensures a consistent form reference. For example, without this change, if the user is on a question then opens the jump screen, the index will be of the question and not of the repeat group.
Currently no text to display, but at least render the UI.
Remove unused imports; add braces; remove unused drawables.
8480bb0
to
77ac22d
Compare
@zestyping I'd appreciate a careful review of this PR if you have a moment! |
@yanokwa Thanks for looping me in! I'm in Dar Es Salaam wrangling map data for a hospital at the moment, so I might not get to this before the call on Friday. I can try to have a look at it on the weekend, if that's not too late. |
I know keeping arrow icons doesn't make sense now as this pr removes expanding/collapsing but maybe we should add another icon to indicate it's a group we can open. This pr adds just a text
what do you think? Maybe one from those: |
@grzesiek2010 I completely agree, and we actually have that in mind for an upcoming PR. I didn't want to put too many separate changes in one PR but we do want to add icons to each type of question to visually represent the question/item type (e.g. repeatable group, date, text, image, etc.). Would you be okay with holding off on that for now? |
collect_app/src/main/java/org/odk/collect/android/logic/HierarchyElement.java
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java
Outdated
Show resolved
Hide resolved
I would prefer having that one with this pr. I don't know how long it will take to review/accept and merge another pr. We can release a new version in the middle so it would be better to add a group icon in this pr. It's not a big deal to do that so if you can please do. |
I think keeping the text is a good idea. More information about what a thing is is helpful.
Update: Looks like you've already thought of that at cooperka#4 Finally, do you have the XLS that generated the XML, might be helpful for the testers. |
Good eye, yes the breadcrumbs will be significantly cleaned up in that upcoming PR. I also just updated the description to include the XLS. |
@cooperka thanks! It looks good to me. @zestyping please take a look when you are free. |
@mmarciniak90 @kkrawczyk123
Everything should work as before apart from repeatable groups which will be displayed in a new way. |
I'll admit I'm a bit disappointed to see the stateful approach remain but I also agree that this is a pragmatic approach. The commit history is really clean. I've only done a quick read through but for what it's worth, I don't have any concerns. |
I agree @lognaturel, I still hope we can get there eventually. The alternative stateless/back-stack approach is a much bigger project than we originally anticipated 😞 It seems to me like something that needs up-front commitment to see it through all the way (one piece at a time) if it's going to be attempted, otherwise we'll create the n+1 problem for form navigation. Hopefully we or someone else will be able to get our hands dirty in the near future. |
@cooperka, I really like the new hierarchy view as it looks very pretty but I have noticed some issues:
@opendatakit-bot unlabel "needs testing" |
Thank you @kkrawczyk123! I'll address those and get back to you. |
Manually restoreInstanceState because onRestoreInstanceState happens too late after onCreate.
This logic was obsolete; probably related to old expand/collapse behavior. setAdapter is also already called by refreshView, so no need for it here. Standardize on 'break' instead of 'return' for consistency.
@kkrawczyk123 can you try again please? I just made a couple fixes (tested and verified locally). |
collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/activities/FormHierarchyActivity.java
Outdated
Show resolved
Hide resolved
Tested with success!! @opendatakit-bot unlabel "needs testing" |
@cooperka Looking forward to the other PRs that build on this one! |
Implements the core behavior described in getodk/roadmap#19. The new buttons will be added in a separate upcoming PR.
Based on branch #2730.
What has been done to verify that this works as intended?
Extensive manual testing with:
jr-template
tag, so it shows a blank child by default)Navigating in and out of nested repeat groups, adding children, editing names, using the back button, jumping back and forth between the FormEditor and FormHierarchy views.
Why is this the best possible solution? Were any other approaches considered?
We considered overhauling
FormHierarchyActivity
to navigate using a more conventional "back stack" instead of usingrefreshView
, but that requires all method calls to be stateless to avoid corrupting the state of earlier activities, whichFormController
currently doesn't support. This simpler solution works within the existing event-based form parsing framework.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This changes the navigational UI so that instead of expanding a dropdown showing the children of a repeatable group, those children are shown on their own on an intermediary screen (sometimes referred to as the "picker"). This intermediary screen will be used in an upcoming PR to display an "add" button to add new items to the repeat group. Overall these changes aim to improve the speed and intuitiveness of navigation.
The behavior changes should be isolated to FormHierarchyActivity, but this activity interacts with the global state via FormController, so regressions can be avoided by ensuring that the state is not mutated in some new unexpected way.
Do we need any specific form for testing your changes? If so, please attach one.
Linked above.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
I was unable to find any existing documentation that needs to be changed (namely here which has screenshots of FormHierarchy but no expandable repeat groups).
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.