Feature/aris/threads beta feedback #5647

ariskotsomitopoulos · 2022-03-28T11:08:47Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Users will be able to provide feedback for threads

Screenshots / GIFs

Before	After	Feedback

Closes #5452

github-actions · 2022-03-28T11:44:53Z

Unit Test Results

110 files +  4 110 suites +4 1m 23s ⏱️ -3s
195 tests +  7 195 ✔️ +  7 0 💤 ±0 0 ❌ ±0
650 runs +28 650 ✔️ +28 0 💤 ±0 0 ❌ ±0

Results for commit a269aa5. ± Comparison against base commit 08476a9.

This pull request removes 2 and adds 9 tests. Note that renamed tests count towards both.

im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ when registering account, then updates state and emits account created event

im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given direct authentication throws, when logging in directly, then returns failure result with original cause
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown fails with content, when logging in directly, then returns success with direct session result
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown fails without content, when logging in directly, then returns well known error
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown throws, when logging in directly, then returns failure result with original cause
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ when logging in directly, then returns success with direct session result
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given has sign in with matrix id sign mode, when handling login or register action fails, then emits error
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given has sign in with matrix id sign mode, when handling login or register action, then logs in directly
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled and registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled, when registering account, then updates state and emits account created event

♻️ This comment has been updated with latest results.

bmarty

Some small remarks

bmarty · 2022-04-05T09:57:10Z

vector-config/src/main/res/values/config-features.xml

+    to the users.
+     -->
+
+    <bool name="feature_threads_beta_feedback_enabled">true</bool>


Since this is a temporary flag, it's OK to keep it like that, but a FeatureFlag would have been better here. I am still writting the doc about it)

bmarty · 2022-04-05T10:00:00Z

vector/src/main/res/values/strings.xml

@@ -1648,6 +1648,10 @@
    <string name="feedback_sent">Thanks, your feedback has been successfully sent</string>
    <string name="feedback_failed">The feedback failed to be sent (%s)</string>
    <string name="give_feedback">Give Feedback</string>
+    <string name="give_feedback_threads">Give Feedback on threads</string>


F -> f ?

Suggested change

<string name="give_feedback_threads">Give Feedback on threads</string>

<string name="give_feedback_threads">Give feedback on threads</string>

Its with capital "F" in the designs

Yes, but the current strategy is rather to avoid capitalization. Can you double check with the product please?

They still want that with Capital F for that specific case, new string resource will be Give Feedback instead of Give Feedback on threads

ok, thanks for double checking.

bmarty · 2022-04-05T10:00:48Z

vector/src/main/java/im/vector/app/features/rageshake/BugReporter.kt

                        ReportType.AUTO_UISI_SENDER,
-                        ReportType.AUTO_UISI           -> bugDescription
+                        ReportType.AUTO_UISI             -> bugDescription
+                        ReportType.THREADS_BETA_FEEDBACK -> "[Element] [threads-feedback] $bugDescription"


Not a blocker but I would have added this case just below SPACE_BETA_FEEDBACK

Refactored!

bmarty · 2022-04-05T10:01:07Z

vector/src/main/java/im/vector/app/features/rageshake/BugReporter.kt

@@ -350,6 +351,7 @@ class BugReporter @Inject constructor(
                            builder.addFormDataPart("label", "android")
                            builder.addFormDataPart("label", "uisi-sender")
                        }
+                        ReportType.THREADS_BETA_FEEDBACK -> builder.addFormDataPart("label", "threads-feedback")


same remark about ordering cases

bmarty · 2022-04-05T10:01:56Z

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

+        app:layout_constraintEnd_toEndOf="parent"
+        android:background="@drawable/bg_shadow_divider"
+        app:layout_constraintBottom_toTopOf="@id/threadsFeedBackConstraintLayout"
+        />


To avoid possible blink, it's better to hide those new views by default.

In this case it's the opposite, to avoid blinking we should turn them on, they are not dynamic. Feedback will be on by default

OK. But it's worth to see the View to quickly vanish when opening the screen (if the bool was set to false), than to see the View to quickly appear.

I see your point, I have something in mind that will be a solution for both

@bmarty A solution would be to use data binding conversions such as:

@BindingConversion fun booleanToVisibility(visible: Boolean) = if(visible) View.VISIBLE else View.GONE

So then we will be able to do in our xml:
android:visibility="@{@bool/myBooleanFlag}"

However this requires to enable data binding for the whole project and introduces some boilerplate, so I will change the default to false !

yeah, that does not worth it. Also in the XML config we could have view style, defining a visibility...

bmarty · 2022-04-05T10:03:54Z

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

+        android:id="@+id/threadsFeedBackConstraintLayout"
+        android:layout_width="0dp"
+        android:layout_height="wrap_content"
+        android:elevation="30dp"


Why do you need an elevation here?

For the shadow, however I enhanced it by adding the divider view so we don't really need that

bmarty · 2022-04-05T10:13:33Z

library/ui-styles/src/main/res/drawable/bg_shadow_divider.xml

+        android:startColor="#00000000"
+        android:endColor="#1A000000"
+        />
+</shape>


Separator on dark theme is maybe not visible enough:

bmarty

Thanks for the update!

ariskotsomitopoulos added 5 commits March 28, 2022 13:53

Add beta feedback for threads

58cc393

Add feature specific configurations

a2e2cdc

Enable threads beta feedback

9eccb9e

Add changelog

b996e0e

Replace hardcoded string

3ba2419

ariskotsomitopoulos requested a review from bmarty March 28, 2022 11:18

ariskotsomitopoulos requested a review from a team March 30, 2022 08:51

ouchadam added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Apr 5, 2022

bmarty reviewed Apr 5, 2022

View reviewed changes

ariskotsomitopoulos added 4 commits April 5, 2022 14:50

Update string resource

b5f8d2c

Format code ordering

aef4cce

Change default threads beta visibility

858623b

Remove unused elevation

a269aa5

bmarty approved these changes Apr 5, 2022

View reviewed changes

bmarty merged commit fac317d into develop Apr 5, 2022

bmarty deleted the feature/aris/threads_beta_feedback branch April 5, 2022 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/aris/threads beta feedback #5647

Feature/aris/threads beta feedback #5647

ariskotsomitopoulos commented Mar 28, 2022 •

edited

Loading

github-actions bot commented Mar 28, 2022 •

edited

Loading

bmarty left a comment

bmarty Apr 5, 2022

bmarty Apr 5, 2022

ariskotsomitopoulos Apr 5, 2022

bmarty Apr 5, 2022

ariskotsomitopoulos Apr 5, 2022

bmarty Apr 5, 2022

bmarty Apr 5, 2022

ariskotsomitopoulos Apr 5, 2022

bmarty Apr 5, 2022

ariskotsomitopoulos Apr 5, 2022

bmarty Apr 5, 2022

ariskotsomitopoulos Apr 5, 2022

bmarty Apr 5, 2022 •

edited

Loading

ariskotsomitopoulos Apr 5, 2022

ariskotsomitopoulos Apr 5, 2022

bmarty Apr 5, 2022

bmarty Apr 5, 2022

ariskotsomitopoulos Apr 5, 2022

bmarty Apr 5, 2022

bmarty left a comment

	<string name="give_feedback_threads">Give Feedback on threads</string>
	<string name="give_feedback_threads">Give feedback on threads</string>

Feature/aris/threads beta feedback #5647

Feature/aris/threads beta feedback #5647

Conversation

ariskotsomitopoulos commented Mar 28, 2022 • edited Loading

Type of change

Content

Screenshots / GIFs

github-actions bot commented Mar 28, 2022 • edited Loading

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

ariskotsomitopoulos commented Mar 28, 2022 •

edited

Loading

github-actions bot commented Mar 28, 2022 •

edited

Loading

bmarty Apr 5, 2022 •

edited

Loading