Skip to content
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

Merged
merged 19 commits into from Jan 13, 2022
Merged

FTUE Use case UI/UX #4927

merged 19 commits into from Jan 13, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Jan 12, 2022

Part of #4585

  • Builds the base UI and UX for the use case screen in the FTUE onboarding flow (behind a feature flag)
  • Fixes the setTextWithColoredPart ignoring the underline argument, the default behaviour is now underline=true to match the previous behaviour

TODO

  • Storing the selection and Integrating with posthog
  • Dark mode icon updates (if signed off)
SCREEN TABLET PORTRAIT TABLET LANDSCAPE SMALL TOP SMALL SCROLLED
Screenshot_20220113_115935 Screenshot_20220113_120252 Screenshot_20220113_120238 Screenshot_20220113_115919 Screenshot_20220113_115925
GIF
use-case-gif

@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Jan 12, 2022
@ouchadam ouchadam mentioned this pull request Jan 12, 2022
6 tasks
@github-actions
Copy link

github-actions bot commented Jan 12, 2022

Unit Test Results

  66 files  ±0    66 suites  ±0   55s ⏱️ -5s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit c3ac60f. ± Comparison against base commit c1d89c5.

♻️ This comment has been updated with latest results.

Copy link
Member

@bmarty bmarty left a 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 {
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

@ouchadam ouchadam Jan 12, 2022

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

Copy link
Member

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)

Copy link
Contributor Author

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(
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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)

Copy link
Contributor Author

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"
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep it's broken...

240 DPI - nexus one
Screenshot_20220113_110500

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b371e24

PR description screenshots updated

android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginBottom="16dp"
android:background="@drawable/bg_login_server_selector"
Copy link
Member

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" />
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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>
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 will do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions
Copy link

github-actions bot commented Jan 13, 2022

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed="21" failures="0" errors="0" skipped="2"
  • [org.matrix.android.sdk.account]
    passed="5" failures="0" errors="0" skipped="2"
  • [org.matrix.android.sdk.internal]
    passed="158" failures="1" errors="0" skipped="38"
  • [org.matrix.android.sdk.ordering]
    passed="16" failures="0" errors="0" skipped="0"
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed="2" failures="0" errors="0" skipped="0"

@ouchadam ouchadam requested a review from bmarty January 13, 2022 13:10
Copy link
Member

@bmarty bmarty left a 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

@ouchadam ouchadam merged commit a208b48 into develop Jan 13, 2022
@ouchadam ouchadam deleted the feature/adm/ftue-usecase branch January 13, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-FTUE Issue is relevant to the first time use project or experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants