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

Updated splash carousel images and copy #4880

Merged
merged 14 commits into from
Jan 17, 2022

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Jan 7, 2022

Part of #4584

1 2 3 4
light_1 light_2 light_3 light_4
dark_1 dark_2 dark_3 dark_4
TABLET PORTRAIT TABLET LANDSCAPE SMALL PHONE MULTI WINDOW MULTI WINDOW
Screenshot_20220114_160815 Screenshot_20220114_160800 Screenshot_20220114_160616 Screenshot_20220114_160730 Screenshot_20220114_160656

private val stringProvider: StringProvider
) {

fun create() = SplashCarouselState(listOf(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the state creation has been extracted out to a dedicated factory as we need to apply some span logic to the terminating .

@@ -39,7 +39,7 @@

<ImageView
android:id="@+id/carousel_item_image"
android:layout_width="wrap_content"
android:layout_width="match_parent"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that we have consistent bounding boxes we can matching the parent (which is constrained by gutters)


private fun Int.colorTerminatingFullStop(@AttrRes color: Int): EpoxyCharSequence {
val string = stringProvider.getString(this)
val fullStop = "."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is incredibly decoupled from the input text to allow languages which don't use full stops to be skipped, maybe a good idea or a terrible one, I'm not sure yet.

Copy link
Member

Choose a reason for hiding this comment

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

It looks acceptable to me like this. Not a big deal anyway.

@ouchadam ouchadam mentioned this pull request Jan 7, 2022
10 tasks
@github-actions
Copy link

github-actions bot commented Jan 7, 2022

Unit Test Results

  66 files  ±0    66 suites  ±0   49s ⏱️ -3s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 1528238. ± Comparison against base commit f025554.

♻️ 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.

Thanks, just some thoughts.


private fun Int.colorTerminatingFullStop(@AttrRes color: Int): EpoxyCharSequence {
val string = stringProvider.getString(this)
val fullStop = "."
Copy link
Member

Choose a reason for hiding this comment

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

It looks acceptable to me like this. Not a big deal anyway.

private fun Int.colorTerminatingFullStop(@AttrRes color: Int): EpoxyCharSequence {
val string = stringProvider.getString(this)
val fullStop = "."
val charSequence = if (string.endsWith(fullStop)) {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering how it will work with RTL languages. Not critical anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PSEUDO RTL
Screenshot_20220110_145700

it doesn't work and the String behaviour is even stranger...

value: ‏‮Own‬‏ ‏‮your‬‏ ‏‮conversions.‬‏
first char: "‏" last char: "‏"
starts with . : false, ends with . : false 
last index of . : 30, first index of . : 30
length: 33

Copy link
Member

Choose a reason for hiding this comment

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

What is "Pseudo RTL"? Even when forcing RTL from the developer option of my phone, is does not reverse the Strings like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they're fake languages for testing locales https://developer.android.com/guide/topics/resources/pseudolocales

Copy link
Member

Choose a reason for hiding this comment

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

TIL. But seems quite not useful if it breaks fun like endsWith.
I prefer to force the app to "Arabic" for instance in the "Choose language" screen.

android:viewportWidth="300"
android:viewportHeight="300">
<group>
<clip-path
Copy link
Member

Choose a reason for hiding this comment

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

pretty sure lint is already complaining about the complexity of those vectors...
Not a strong opinion neither, but probably quite bad for the performance than classical png/webp images

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as these are one time use assets and they're quite complex I'll covert to .png

Copy link
Contributor Author

@ouchadam ouchadam Jan 11, 2022

Choose a reason for hiding this comment

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

<string name="ftue_auth_carousel_3_title">Connect with anyone.</string>
<string name="ftue_auth_carousel_3_body">Element works with all Matrix-based apps and can even bridge into proprietary messengers.</string>
<string name="ftue_auth_carousel_3_title">Secure Messaging.</string>
<string name="ftue_auth_carousel_3_body">No phone number required, so you don\'t have to share those details with the outside world. No ads, no tracking or datamining.</string>
Copy link
Member

Choose a reason for hiding this comment

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

Is "no tracking" true regarding analytics? CC @daniellekirkwood

Copy link
Contributor

@daniellekirkwood daniellekirkwood Jan 11, 2022

Choose a reason for hiding this comment

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

👀 Good catch, @bmarty

Even though we don't track without explicit consent from the user we'd like to change the content to remove the reference to tracking, that way it's super clear.

"No ads or datamining."

@ouchadam Is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good to me 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Base automatically changed from feature/adm/automatic-splash-carousel-transitions to develop January 11, 2022 11:14
@ouchadam ouchadam force-pushed the feature/adm/carousel-images-update branch from a0bf2ed to 0e3068e Compare January 11, 2022 11:29
@@ -10,4 +10,6 @@
<!-- Value MUST have 4 letters and MUST be in this list: https://www.unicode.org/iso15924/iso15924-codes.html. Example: "Arab", "Cyrl", "Latn", etc. -->
<string name="resources_script">Latn</string>

<string name="cut_the_slack_from_teams">Cut the slack from teams.</string>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to put it in this file and add the translatable="false" attribute

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.

updated 2958644

@ouchadam ouchadam force-pushed the feature/adm/carousel-images-update branch 2 times, most recently from 2958644 to 27377d3 Compare January 11, 2022 16:40
@ouchadam
Copy link
Contributor Author

keeping as a draft until we finalise the carousel copy

@ouchadam ouchadam force-pushed the feature/adm/carousel-images-update branch from 27377d3 to 774bed6 Compare January 14, 2022 09:08
@github-actions
Copy link

github-actions bot commented Jan 14, 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="23" failures="1" errors="0" skipped="3"
  • [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 marked this pull request as ready for review January 14, 2022 16:12
@ouchadam ouchadam requested a review from bmarty January 14, 2022 16:14
@@ -35,7 +35,7 @@ abstract class SplashCarouselItem : VectorEpoxyModel<SplashCarouselItem.Holder>(

holder.view.setBackgroundResource(item.pageBackground)
holder.image.setImageResource(item.image)
holder.title.setText(item.title)
holder.title.text = item.title.charSequence
Copy link
Member

Choose a reason for hiding this comment

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

hopefully there is not Emoji there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 we should be okay as we've using the EpoxyCharSequence and there's no setTextFuture 🤞

<string name="ftue_auth_carousel_2_body">Element lets you choose where you messages are stored, keeping you in control of your data.</string>
<string name="ftue_auth_carousel_3_title">Connect with anyone.</string>
<string name="ftue_auth_carousel_3_body">Element works with all Matrix-based apps and can even bridge into proprietary messengers.</string>
<string name="ftue_auth_carousel_2_body">Element lets you choose where your messages are stored, keeping you in control of your data.</string>
Copy link
Member

@bmarty bmarty Jan 14, 2022

Choose a reason for hiding this comment

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

We should not use hard-coded Element in the string. Use ${app_name} and add template to the string key. (see other example in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very good point, will update 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

57ccf2d

also marks the previous strings for removal

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.

One last remark, sorry.

@ouchadam ouchadam force-pushed the feature/adm/carousel-images-update branch 2 times, most recently from 8ceafb4 to f389e28 Compare January 14, 2022 17:37
@ouchadam ouchadam force-pushed the feature/adm/carousel-images-update branch from f389e28 to 1528238 Compare January 17, 2022 09:27
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 for the update

@ouchadam ouchadam merged commit 256929b into develop Jan 17, 2022
@ouchadam ouchadam deleted the feature/adm/carousel-images-update branch January 17, 2022 15:11
@ouchadam ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Mar 3, 2022
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.

3 participants