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.

Sign up for GitHub

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

Jump to bottom

Enabling FTUE Use Case #5106

Merged
merged 5 commits into from
Feb 3, 2022
Merged

Enabling FTUE Use Case #5106

merged 5 commits into from
Feb 3, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Jan 31, 2022 •
edited
Loading

#4585 Enables the FTUE Use case screen by default

LIGHT DARK
use-case-light use-case-dark
SMALL DEVICE TOP SMALL DEVICE BOTTOM TABLET PORTRAIT TABLET LANDSCAPE
Screenshot_20220202_184858 Screenshot_20220202_184903 Screenshot_20220202_184834 Screenshot_20220202_184847

@ouchadam ouchadam added X-Needs-Design May require input from the design team Z-FTUE Issue is relevant to the first time use project or experience X-Needs-Product Issue needs input from Product team labels Jan 31, 2022
@github-actions
Copy link

github-actions bot commented Jan 31, 2022 •
edited
Loading

Unit Test Results

  76 files  ±0    76 suites  ±0   1m 1s ⏱️ -2s
143 tests ±0  143 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
448 runs  ±0  448 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 63a937c. ± Comparison against base commit cd6ba52.

♻️ This comment has been updated with latest results.

bmarty
bmarty reviewed Jan 31, 2022
View reviewed changes
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.

Thanks. I think the allScreensTest has to be updated, no?

@ouchadam
Copy link
Contributor Author

@bmarty updating the test caught matrix-org/matrix-analytics-events#20

@github-actions
Copy link

github-actions bot commented Jan 31, 2022 •
edited
Loading

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    passed=
  • [org.matrix.android.sdk.account]
    passed=
  • [org.matrix.android.sdk.internal]
    passed=
  • [org.matrix.android.sdk.ordering]
    passed=
  • [org.matrix.android.sdk.PermalinkParserTest]
    passed=

@bmarty
Copy link
Member

bmarty commented Jan 31, 2022

@bmarty updating the test caught matrix-org/matrix-analytics-events#20

I do not see the link between our sanity test and the analytics. Can you enlighten me please?

@ouchadam
Copy link
Contributor Author

ouchadam commented Feb 1, 2022

@bmarty the tests were using the back flow and resetting the identity tracking with "ftueUseCase" = null which turned out to remove the property from the map which then resulted in a crash as the identify must be called with either a valid analytics id or non empty properties

@amshakal
Copy link

amshakal commented Feb 1, 2022

Hello! It's looking good. Just a few small comments:

  1. For dark mode, can we change the colour of 'Not sure yet? You can' and 'looking to join an exitsing server?' to A9B2BC? since it's not the most important information on the screen, id reserve our secondary grey colour for this.
  2. Similarly for light mode, can we change the colour of the items mentioned above to 737D8C?
  3. Can we semi bold the button options - Friends and family, team and communities?
  4. For the outline of these three buttons, Friends and family, team and communities, can we change the outline stroke to 6F7882?

TY!

@ouchadam ouchadam mentioned this pull request Feb 1, 2022
Merged
@ouchadam
Copy link
Contributor Author

ouchadam commented Feb 1, 2022 •
edited
Loading

with fixes applied cc @amshakal

BEFORE AFTER
Screenshot_20220131_142327 Screenshot_20220201_175627
Screenshot_20220131_142312 Screenshot_20220201_175641

We spoke internally and will keep the same option border colours for the time being as they match the existing server selection screen

@ouchadam ouchadam mentioned this pull request Feb 1, 2022
Closed
6 tasks
@kojid0
Copy link
Contributor

kojid0 commented Feb 1, 2022 •
edited
Loading

@ouchadam
I think you missed the third bullet point of her (semi bold the button options).
Also, I'd prefer to have "You can" not be clickable, only "skip this question" (have gone through some design classes + i guess it's also what she meant)

bmarty
bmarty reviewed Feb 1, 2022
View reviewed changes
vector/src/main/res/layout/fragment_ftue_auth_use_case.xml
@@ -79,7 +79,7 @@

<TextView
android:id="@+id/useCaseOptionOne"
style="@style/Widget.Vector.TextView.Subtitle"
style="@style/Widget.Vector.TextView.Subtitle.Medium"
Copy link
Member

Choose a reason for hiding this comment

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

Note that #3907 is a thing and I have still not found a good solution for it. So basically this change has no effect (I think), which does not mean that we should not do it.

Copy link
Contributor Author

@ouchadam ouchadam Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see... the figma typography is a little misleading here then 😅 Seems like semi bold is the correct weighting but I'm not sure if we have that weight in our styles

@ouchadam
Copy link
Contributor Author

ouchadam commented Feb 1, 2022 •
edited
Loading

@kojid0 create catch! I'll update and sync with @amshakal about the font weight as it's set to medium figma and android doesn't currently have any examples of semi bold

👍 for separating the you can and skip this question, the designs have also been updated (or I imagined them the wrong way originally 🤔 )

bmarty
bmarty reviewed Feb 2, 2022
View reviewed changes
vector/src/main/res/values/strings.xml
<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_skip">Not sure yet? You can %s</string>
<string name="ftue_auth_use_case_skip_partial">skip this question</string>
Copy link
Member

Choose a reason for hiding this comment

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

Happy to see that your plan to put the new strings at a temporary location to avoid making Weblate handle them is useful 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.

same! it does mean there's a slight delay with when the translations can be done but it does save us doing multiple redundant translations

bmarty
bmarty approved these changes Feb 2, 2022
View reviewed changes
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!

@ouchadam ouchadam force-pushed the feature/adm/enable-use-case-feature branch from 53052b2 to 2066d3b Compare February 2, 2022 18:51
@ouchadam
Copy link
Contributor Author

ouchadam commented Feb 2, 2022 •
edited
Loading

PR rebased on develop with the font weight fix #5135

  • screenshots updated

bmarty
bmarty approved these changes Feb 2, 2022
View reviewed changes
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!

@ouchadam ouchadam force-pushed the feature/adm/enable-use-case-feature branch from 2066d3b to 364a51a Compare February 3, 2022 11:27
@ouchadam ouchadam force-pushed the feature/adm/enable-use-case-feature branch from 364a51a to 63a937c Compare February 3, 2022 11:28
@ouchadam ouchadam merged commit dbfd7e6 into develop Feb 3, 2022
@ouchadam ouchadam deleted the feature/adm/enable-use-case-feature branch February 3, 2022 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmarty bmarty bmarty approved these changes

@amshakal amshakal amshakal approved these changes

@daniellekirkwood daniellekirkwood daniellekirkwood approved these changes

Assignees
No one assigned
Labels
X-Needs-Design May require input from the design team X-Needs-Product Issue needs input from Product team Z-FTUE Issue is relevant to the first time use project or experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ouchadam @bmarty @amshakal @kojid0 @daniellekirkwood