-
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
Rework quit dialog #5614
Rework quit dialog #5614
Conversation
collect_app/src/androidTest/java/org/odk/collect/android/regression/FillBlankFormTest.java
Outdated
Show resolved
Hide resolved
android:text="@string/discard_changes" | ||
app:icon="@drawable/ic_delete" | ||
app:iconGravity="textStart" | ||
app:iconSize="18dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to set this. I've filed an issue over at the Material Components repo.
app:layout_constraintStart_toStartOf="parent" | ||
app:layout_constraintTop_toBottomOf="@id/discard_changes" /> | ||
|
||
<com.google.android.material.button.MaterialButton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised I'd never run into this before, but it's currently not possible to change the style of a view programmatically (or after it's inflated more accurately). This means having duplicated views for the "Keep editing" button.
collect_app/src/main/java/org/odk/collect/android/formentry/QuitFormDialog.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formentry/QuitFormDialog.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formentry/QuitFormDialog.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formentry/QuitFormDialog.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formentry/saving/FormSaveViewModel.java
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formentry/FormSessionRepository.kt
Show resolved
Hide resolved
@seadowg Please fix conflicts and rebase onto master. |
I found one issue with rotating the screen Steps to reproduce:
XRecorder_21062023_152335.mp4 |
We only talked about it being "Save form", but it would be nice for it to switch to "Save changes" like you described. We can maybe add that another time. |
I can't reproduce this. Are you seeing it every time on multiple devices? |
Yes, on Pixel 3a and Pixel 6a, but you need to have both Save as draft option turned on in the Access control -> Form Entry Setting, otherwise the buttons works correctly |
Also there is something wrong with Save as Draft in Access control -> Form Entry Setting.
XRecorder_21062023_155311.mp4 |
Is just the default setup?
Ah! Looks like I've used the wrong setting. Will fix. |
yep |
@srujner I'm using those settings (demo project and All Widgets form), and I'm not able to reproduce. Could you try an emulator for me? What Android version are those phones using? |
@seadowg Android 12 and 13 I tried on Emulator on Pixel 5 device with Android 12 and I was also able to reproduce it there. Update |
Managed to reproduce on a Pixel 5 Android 13 emulator. No idea what's going on here, but will look into it. |
…hat's already saved
@srujner that's fixed now. I've just made the dialog scrollable so that the layout won't fall apart in landscape on thinner phones. |
Tested with Success! Verified on device with Android 8.1, 10 Verified cases:
|
Tested with Success! Verified on device with Android 12, 13 |
Closes #5490
There's a few differences in the implemented dialog and the mock-ups referenced in the issue (dialog title font weight, button text size etc). I've discussed this with @alyblenkin and as these should all be resolved by #5607 and #5567, we'll wait to do those next release rather than trying to poly fill solutions here.
What has been done to verify that this works as intended?
New (and existing) tests.
Why is this the best possible solution? Were any other approaches considered?
Not a lot to discuss here. The code mostly follows the structure we already had in place. I'll highlight anything of interest with inline comments.
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?
Quitting new and draft forms is the key thing to look at here. It's also important to check these flows with different access control settings (especially the "Save as draft" setting) enabled and disabled. As with the old quit dialog, this one will not remain on screen during configuration changes (like rotations or unfolds for example).
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.