-
Notifications
You must be signed in to change notification settings - Fork 731
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 Splash carousel MVP #4727
FTUE Splash carousel MVP #4727
Conversation
@Inject lateinit var vectorPreferences: VectorPreferences | ||
@Inject lateinit var vectorFeatures: VectorFeatures | ||
@Inject lateinit var carouselController: SplashCarouselController | ||
@Inject lateinit var stringProvider: StringProvider |
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.
Why not injected in the constructor?
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.
(Also a stringProvider here is a bit useless)
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.
wasn't sure if we wanted to use it for consistency (can be helpful if the string provider ends up with extra behaviour)
didn't realise we had setup fragment constructor injection, will try it
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.
1- You are directly using getString()
later in the file :)
2- Cool thanks
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 update this file to explain how to generate webp image and in which case? |
6846cf2
to
4d02621
Compare
fa6f098
to
9a71aea
Compare
4d02621
to
8814cd4
Compare
9a71aea
to
5fafbff
Compare
854953e
to
801a02e
Compare
5fafbff
to
cb88bfd
Compare
801a02e
to
6fbf8fa
Compare
bb356ba
to
b5501a2
Compare
} | ||
|
||
class DefaultVectorFeatures : VectorFeatures { | ||
override fun onboardingVariant(): VectorFeatures.OnboardingVariant = BuildConfig.ONBOARDING_VARIANT | ||
override fun isAlreadyHaveAccountSplashEnabled() = true | ||
override fun isSplashCarouselEnabled() = false |
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.
whilst the carousel is still WIP it's not enabled by default for the FTUE_AUTH
variant
enum class OnboardingVariant { | ||
LEGACY, | ||
LOGIN_2, | ||
FTUE_AUTH | ||
} | ||
|
||
enum class NotificationSettingsVersion { |
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.
this was unused
b5501a2
to
cb1baea
Compare
- also coverts the carousel pages to constraint layout and makes use of gutters to provide percentage based sizing
…ctor for project consistent
…e have the final images before creating separate densities
cb1baea
to
7ad0e25
Compare
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 thanks. Just a few minor remarks
- right clicking the image file within the project file explorer | ||
- select `Convert to WebP` | ||
|
||
https://developer.android.com/studio/write/convert-webp |
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!
views.splashCarousel.adapter = carouselController.adapter | ||
TabLayoutMediator(views.carouselIndicator, views.splashCarousel) { _, _ -> }.attach() | ||
carouselController.setData(SplashCarouselState( | ||
items = listOf( |
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.
Since this state is static, maybe move all this to the constructor of SplashCarouselState
?
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.
<gradient | ||
android:endColor="#3372C7DA" | ||
android:startColor="#33BBE7CF" /> | ||
</shape> |
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 do not have a strong opinion on it, but maybe move the new drawables to the module ui-styles
? The webp images could stay in the vector
project. IDK...
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.
sounds good to me 👍
785a142
<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_4_title">Cut the slack from teams.</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.
Wow :p
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 very subtle 😂
As a side remark, I am wondering if we could have some sort of parallax effect for the background gradient. It looks a bit too simple like this. |
it's definitely something we could iterate on |
Part of #4584
TODO