Create submission consent screen (QR code flow) (EXPOSUREAPP-3676) #1712
Conversation
…e/3731-submission-consent-screen # Conflicts: # Corona-Warn-App/src/main/res/layout/fragment_submission_qr_code_info.xml # Corona-Warn-App/src/main/res/values-bg/strings.xml # Corona-Warn-App/src/main/res/values-en/strings.xml # Corona-Warn-App/src/main/res/values-pl/strings.xml # Corona-Warn-App/src/main/res/values-ro/strings.xml # Corona-Warn-App/src/main/res/values-tr/strings.xml # Corona-Warn-App/src/main/res/values/strings.xml
|
Hi @chiljamgossow - I am just wondering why this PR includes files for languages other than the source language German? These are normally update by deliveries from translation. |
chiljamgossow
changed the title
Hi @janetback I deleted a screen and also removed the strings that are now unused. |
|
For my taste looks nice in general. @janetback and @SabineLoss : I would suggest to move the text "Die App kann nicht gleichzeitg mehrere Tests verwalten" up one icon/bullet point, directly following "Scannen Sie nur Ihren eigenen Test". (There should also be a "." behind "Test" for consistency.) With regard to content it maybe fits better there. |
| style="@style/buttonPrimary" | ||
| android:layout_width="@dimen/match_constraint" | ||
| android:layout_height="wrap_content" | ||
| android:text="@string/submission_positive_other_warning_button" |
There was a problem hiding this comment.
maybe rename the string resources so that they represent the usage place more accurately
There was a problem hiding this comment.
renamed to submission_accept_button
| android:layout_height="wrap_content" | ||
| android:text="@string/submission_positive_other_warning_button" | ||
| android:textAllCaps="true" | ||
| android:onClick="@{ () -> viewModel.onConsentButtonClick()}" |
There was a problem hiding this comment.
could we not inject the viewModel and instead just set the listener in fragment code?
There was a problem hiding this comment.
This is possible. I decided to use data binding to keep the fragment clean
| } | ||
|
|
||
| @Test | ||
| fun testBackPressButton() { |
There was a problem hiding this comment.
is this actually the back button being tested?
There was a problem hiding this comment.
Thanks for spotting this. I changed this.
|
Hi @vaubaehn, thanks for your feedback. We will add the missing '.' and take your suggestions into account for future improvements. @chiljamgossow - as discussed, could you please add the missing punctuation? Thanks. |
done |
review comments
test code
| override fun onViewCreated(view: View, savedInstanceState: Bundle?) { | ||
| super.onViewCreated(view, savedInstanceState) | ||
| binding.viewModel = viewModel | ||
| binding.lifecycleOwner = this |
There was a problem hiding this comment.
binding.lifecycleOwner = this should be replaced with this.viewLifecycleOwner
(can lead to fragment issues e.g. while navigating)
It's also little bit redundant because you already use viewBindingLazy above.
So to avoid this bug either adjust the lifecycleOwner line or simply remove it ⛄
There was a problem hiding this comment.
I removed it. I missed that it is already applied.
|
Kudos, SonarCloud Quality Gate passed! |
|
Hi @janetback , thanks for your kind reply. From the user's perspective, I also liked your approach from the later erased comment for its clarity, but full understanding that it's difficult to find the balance between wording and length, without breaking the layouts 😉 Thanks for your important contributions to this big project 👍 -> this goes also to @SabineLoss |
|
Hi @vaubaehn , yes, we are definitely keeping the proposal that I then erased - we are still hoping to be able to implement it at a slightly later date. |
chiljamgossow
deleted the
feature/3676-create-submission-consent-screen
branch
Creates new screen for consent to submission of keys in QR code flow
Removes old info screen and dialog
Updates navigation
Link to data privacy screen
Move legal text
Still missing: correct icons -> moved to new ticket 3972