-
Notifications
You must be signed in to change notification settings - Fork 784
FTUE Use case UI/UX #4927
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
FTUE Use case UI/UX #4927
Conversation
… it's set - updates the default value to true and that was the existing intentional behaviour
… at the same time for visibility
… act on the use caseselection)
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.
Nice work, some small remarks. Thanks!
@@ -31,6 +31,15 @@ sealed class OnboardingAction : VectorViewModelAction { | |||
|
|||
data class UpdateServerType(val serverType: ServerType) : OnboardingAction() | |||
data class UpdateHomeServer(val homeServerUrl: String) : OnboardingAction() | |||
data class UpdateUseCase(val useCase: UseCase) : OnboardingAction() { | |||
enum class UseCase { |
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.
Nit: Other enums (SignMode
, etc.) are declared in their own file, maybe do the same, this file is big enough.
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.
extracted 5d0c55b
@@ -441,6 +457,11 @@ class OnboardingViewModel @AssistedInject constructor( | |||
} | |||
} | |||
|
|||
private fun handleUpdateUseCase() { | |||
// TODO act on the use case selection |
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.
Yeah, maybe just store it in a data store for now, for future usage in the app?
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 planning to store and apply to the posthog identity in the next PR to keep this PR a little smaller, but I'm also happy to do the local storing here to avoid the TODO
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.
Ok, it's fine to do it in another PR. About analytics you do not have the user consent yet (in the current impl)
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.
that's fine, we'll store selection locally and when consent is granted we'll retrieve the stored value and apply as an identity property in the analytics init
views.useCaseOptionThree.setUseCase(R.string.ftue_auth_use_case_option_three, UseCase.COMMUNITIES) | ||
|
||
val partial = getString(R.string.ftue_auth_use_case_skip_partial) | ||
views.useCaseSkip.setTextWithColoredPart( |
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.
You can use (I think) the other version of this fun, which will take the string resource R.string.ftue_auth_use_case_skip_partial
and R.string.ftue_auth_use_case_skip, partial
as parameters. Shorter code.
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.
works great! c3a7f6e
} | ||
|
||
override fun resetViewModel() { | ||
// Nothing to do |
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.
It should reset the previous answer from the user for this question (if it's in the state)
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.
f3cc7e9 relies on the same TODO for storing the use case selection, we don't need to store the selection in the onboarding state
android:id="@+id/useCaseHeaderIcon" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginTop="52dp" |
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 do not have a scrollview here, I am afraid the whole screen will render badly on small screens (did not check)
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.
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.
PR description screenshots updated
android:layout_width="0dp" | ||
android:layout_height="wrap_content" | ||
android:layout_marginBottom="16dp" | ||
android:background="@drawable/bg_login_server_selector" |
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.
Thanks for reusing this drawable!
app:layout_constraintBottom_toTopOf="@id/contentFooterSpacing" | ||
app:layout_constraintEnd_toEndOf="@id/useCaseGutterEnd" | ||
app:layout_constraintStart_toStartOf="@id/useCaseGutterStart" | ||
app:layout_constraintTop_toBottomOf="@id/useCaseOptionThree" /> |
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.
maybe add a tools:text
attribute for a better preview of the layout
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.
app:layout_constraintStart_toStartOf="@id/useCaseGutterStart" | ||
app:layout_constraintTop_toBottomOf="@id/contentFooterSpacing" /> | ||
|
||
<TextView |
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.
Better to use a Button
here.
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.
47c9e75 also updated the splash I already know this
to use a button as well
<string name="ftue_auth_use_case_skip">Not sure yet? %s</string> | ||
<string name="ftue_auth_use_case_skip_partial">You can skip this question</string> | ||
<string name="ftue_auth_use_case_join_existing_server">Looking to join an existing server?</string> | ||
<string name="ftue_auth_use_case_connect_to_server">Connect to server</string> |
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.
It's OK, but you were thinking about adding new strings to another file waiting for final copy.
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.
💯 will do
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.
…es to avoid translations before they're signed off
Matrix SDKIntegration Tests Results:
|
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.
LGTM, thanks for the update! Please merge it when you want to
Part of #4585
setTextWithColoredPart
ignoring theunderline
argument, the default behaviour is nowunderline=true
to match the previous behaviourTODO