applayout release experience #7003

fedrunov · 2022-09-05T07:40:00Z

Type of change

Feature
Bugfix
Technical
Other :

Content

we want to onboard users to new app layout

Motivation and context

closes #6909

Screenshots / GIFs

there are different margins for tall and short screens

Tall	Short

Light	Dark	Black

bmarty

A few first remarks, I will test it when the PR will be updated.

library/ui-strings/src/main/res/values/strings.xml

vector/src/main/java/im/vector/app/features/home/HomeActivityViewModel.kt

vector/src/main/java/im/vector/app/features/home/room/list/home/release/ReleaseNotesFragment.kt

bmarty · 2022-09-05T09:02:33Z

...main/java/im/vector/app/features/home/room/list/home/release/ReleaseNotesPreferencesStore.kt

+        context.dataStore.edit { settings ->
+            settings[isAppLayoutOnboardingShown] = isShown
+        }
+    }


Are you sure the Activity will not be show for user launching Element for the first time? This is a requirement of the issue. I do not see the logic here.

You can (maybe) test the nullability of HomeActivityViewState.authenticationDescription https://github.com/vector-im/element-android/blob/main/vector/src/main/java/im/vector/app/features/home/HomeActivityViewState.kt#L25 to have the info.

fixed - https://github.com/vector-im/element-android/pull/7003/files#diff-7153f5b6cf4c510293d53c73474e854a3a84e6003c88341472c0808377bf3462R121

also fixed problem when this value was null after login - https://github.com/vector-im/element-android/pull/7003/files#diff-31d1cb8d12c34d852a52405908bc8f1a427a1f08485a662527a6ff5c11e7053eR630

also fixed problem when this value was null after login - https://github.com/vector-im/element-android/pull/7003/files#diff-31d1cb8d12c34d852a52405908bc8f1a427a1f08485a662527a6ff5c11e7053eR630

I think this fixes the issue, but I am wondering if this is the best place to set the state.selectedAuthenticationState. Not blocking this PR, but maybe @ouchadam can have a quick look later on this change.

for context, the reason why this was null previously is because the value was only being used to track signup events in the HomeActivityViewModel

for the change, I think it's fine, we could eventually remove the selectedAuthenticationState parameter and instead rely on updating the state before calling onSessionCreated

...or/src/main/java/im/vector/app/features/home/room/list/home/release/ReleaseNotesViewModel.kt

vector/src/main/java/im/vector/app/features/popup/PopupAlertManager.kt

vector/src/main/res/layout/bottom_sheet_release_notes.xml

bmarty

A few more remarks on the layout

vector/src/main/res/layout/item_release_carousel.xml

vector/src/main/java/im/vector/app/features/home/HomeActivityViewModel.kt

ericdecanini

Benoit has said most already. The only thing I can add is maybe you can change the status bar color in this new activity to match the background to look a bit nicer. Consider it optional

vector/src/main/java/im/vector/app/features/home/room/list/home/release/ReleaseCarouselItem.kt

...or/src/main/java/im/vector/app/features/home/room/list/home/release/ReleaseNotesViewModel.kt

...main/java/im/vector/app/features/home/room/list/home/release/ReleaseNotesPreferencesStore.kt

bmarty

Thanks for the update. Some last remarks.

library/ui-strings/src/main/res/values/strings.xml

vector/src/main/java/im/vector/app/features/home/HomeActivityViewModel.kt

vector/src/main/java/im/vector/app/features/home/room/list/home/release/ReleaseNotesFragment.kt

Co-authored-by: Benoit Marty <[email protected]>

…iewModel.kt Co-authored-by: Benoit Marty <[email protected]>

…erly

…prefix

ElementBot · 2022-09-06T06:11:33Z

	Warnings
⚠️	vector/src/main/AndroidManifest.xml#L220 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L220 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L223 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L223 - Consider splitting data tag into multiple tags with individual attributes to avoid confusion
⚠️	vector/src/main/AndroidManifest.xml#L277 - Expecting `android:screenOrientation="unspecified"` or `"fullSensor"` for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices
⚠️	vector/src/main/AndroidManifest.xml#L277 - Expecting `android:screenOrientation="unspecified"` or `"fullSensor"` for this activity so the user can use the application in any orientation and provide a great experience on Chrome OS devices
⚠️	vector/src/main/AndroidManifest.xml#L286 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L286 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L293 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L293 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L299 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L299 - Attribute `supportsPictureInPicture` is only used in API level 24 and higher (current min is 21)
⚠️	vector/src/main/AndroidManifest.xml#L414 - Exported receiver does not require permission
⚠️	vector/src/main/AndroidManifest.xml#L414 - Exported receiver does not require permission
⚠️	vector/src/main/res/layout/bottom_sheet_release_notes.xml#L8 - Possible overdraw: Root element paints background `?colorSurface` with a theme that also paints a background (inferred theme is `@android:style/Theme.Holo`)
⚠️	vector/src/main/res/layout/bottom_sheet_release_notes.xml#L8 - Possible overdraw: Root element paints background `?colorSurface` with a theme that also paints a background (inferred theme is `@android:style/Theme.Holo`)

Generated by 🚫 dangerJS against eac9371

sonarqubecloud · 2022-09-06T06:35:46Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

bmarty

LGTM, thanks!

applayout release experience

f8ec268

fedrunov requested review from bmarty and ericdecanini September 5, 2022 07:40

bmarty reviewed Sep 5, 2022

View reviewed changes

vector/src/main/res/layout/item_release_carousel.xml Outdated Show resolved Hide resolved

vector/src/main/res/layout/item_release_carousel.xml Outdated Show resolved Hide resolved

vector/src/main/res/layout/item_release_carousel.xml Outdated Show resolved Hide resolved

bmarty reviewed Sep 5, 2022

View reviewed changes

vector/src/main/java/im/vector/app/features/home/HomeActivityViewModel.kt Outdated Show resolved Hide resolved

bmarty reviewed Sep 5, 2022

View reviewed changes

vector/src/main/java/im/vector/app/features/home/HomeActivityViewModel.kt Show resolved Hide resolved

ericdecanini approved these changes Sep 5, 2022

View reviewed changes

vector/src/main/java/im/vector/app/features/home/room/list/home/release/ReleaseCarouselItem.kt Show resolved Hide resolved

...or/src/main/java/im/vector/app/features/home/room/list/home/release/ReleaseNotesViewModel.kt Outdated Show resolved Hide resolved

bmarty reviewed Sep 5, 2022

View reviewed changes

...main/java/im/vector/app/features/home/room/list/home/release/ReleaseNotesPreferencesStore.kt Show resolved Hide resolved

bmarty mentioned this pull request Sep 5, 2022

App layout Home screen empty states #7007

Merged

4 tasks

NIkita Fedrunov added 5 commits September 5, 2022 15:56

review fixes

74fd14a

fixed missing auth description + flag screen as shown after auth flow

f2309af

rtl comment for string

d986fed

logs removed

e29687f

tools sources added for carousel item layout

108e1b8

fedrunov requested a review from bmarty September 5, 2022 14:38

don't show release notes if app layout isn't enabled

fc265dd

bmarty reviewed Sep 5, 2022

View reviewed changes

fedrunov and others added 7 commits September 5, 2022 17:11

Update library/ui-strings/src/main/res/values/strings.xml

744d212

Co-authored-by: Benoit Marty <[email protected]>

Update vector/src/main/java/im/vector/app/features/home/HomeActivityV…

7515b77

…iewModel.kt Co-authored-by: Benoit Marty <[email protected]>

remvoved +id reference

d18f3cf

flag change is set before posting action to ensure it's executed prop…

6a9284e

…erly

some lint fixes

1153297

removed duplicated illustrations + illustrations renamed to have ill …

1afd10e

…prefix

fixed tests to expect selected auth state after auth

eac9371

fedrunov added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Sep 6, 2022

bmarty approved these changes Sep 6, 2022

View reviewed changes

fedrunov merged commit 7cc7444 into develop Sep 6, 2022

fedrunov deleted the feature/nfe/app_layout_release_experience branch September 6, 2022 09:32

bmarty mentioned this pull request Sep 7, 2022

Fix ReleaseNotesActivity displayed after login. #7061

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applayout release experience #7003

applayout release experience #7003

fedrunov commented Sep 5, 2022

bmarty left a comment

bmarty Sep 5, 2022

bmarty Sep 5, 2022

fedrunov Sep 5, 2022

bmarty Sep 5, 2022

ouchadam Sep 12, 2022

bmarty left a comment

ericdecanini left a comment

bmarty left a comment

ElementBot commented Sep 6, 2022

sonarqubecloud bot commented Sep 6, 2022

bmarty left a comment

applayout release experience #7003

applayout release experience #7003

Conversation

fedrunov commented Sep 5, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

bmarty left a comment

Choose a reason for hiding this comment

bmarty Sep 5, 2022

Choose a reason for hiding this comment

bmarty Sep 5, 2022

Choose a reason for hiding this comment

fedrunov Sep 5, 2022

Choose a reason for hiding this comment

bmarty Sep 5, 2022

Choose a reason for hiding this comment

ouchadam Sep 12, 2022

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ericdecanini left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ElementBot commented Sep 6, 2022

sonarqubecloud bot commented Sep 6, 2022

bmarty left a comment

Choose a reason for hiding this comment