"Create poll" UI #1143

julioromano · 2023-08-24T11:57:46Z

NB: This is missing analytics, which will be added once matrix-org/matrix-analytics-events#85 is merged.

Closes element-hq/element-meta#2011

github-actions · 2023-08-24T12:10:11Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/KJzUnv

codecov · 2023-08-25T12:28:00Z

Codecov Report

Patch coverage: 72.82% and project coverage change: +0.22% 🎉

Comparison is base (5a85459) 56.87% compared to head (10d07d1) 57.09%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1143      +/-   ##
===========================================
+ Coverage    56.87%   57.09%   +0.22%     
===========================================
  Files         1039     1044       +5     
  Lines        26812    27092     +280     
  Branches      5544     5592      +48     
===========================================
+ Hits         15248    15469     +221     
- Misses        9168     9201      +33     
- Partials      2396     2422      +26

Files Changed	Coverage Δ
...es/poll/impl/create/DefaultCreatePollEntryPoint.kt	`0.00% <0.00%> (ø)`
...ges/impl/messagecomposer/AttachmentsBottomSheet.kt	`41.17% <33.33%> (-1.05%)`	⬇️
...ssages/impl/messagecomposer/MessageComposerView.kt	`53.12% <33.33%> (-0.21%)`	⬇️
...es/messages/impl/actionlist/ActionListPresenter.kt	`84.12% <36.36%> (-10.11%)`	⬇️
...ndroid/features/poll/impl/create/CreatePollView.kt	`50.00% <50.00%> (ø)`
...ent/android/features/messages/impl/MessagesView.kt	`59.09% <66.66%> (+0.11%)`	⬆️
...s/impl/messagecomposer/MessageComposerPresenter.kt	`90.38% <66.66%> (-0.95%)`	⬇️
...d/features/poll/impl/create/CreatePollPresenter.kt	`78.46% <78.46%> (ø)`
...droid/features/poll/impl/create/CreatePollState.kt	`90.90% <90.90%> (ø)`
...atures/poll/impl/create/CreatePollStateProvider.kt	`94.11% <94.11%> (ø)`
... and 5 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmarty

Nice work, a few remarks and one (or 2) blocking points.
Also maybe add the create poll flow in the Maestro test (can be done later of course).

EDIT: Creating a poll worked :)

bmarty · 2023-08-29T09:32:53Z

...kotlin/io/element/android/features/messages/impl/messagecomposer/MessageComposerPresenter.kt

@@ -83,6 +83,11 @@ class MessageComposerPresenter @Inject constructor(
            canShareLocation.value = featureFlagService.isFeatureEnabled(FeatureFlags.LocationSharing)
        }

+        val canCreatePoll = remember { mutableStateOf(false) }
+        LaunchedEffect(Unit) {
+            canCreatePoll.value = featureFlagService.isFeatureEnabled(FeatureFlags.Polls)


Maybe update the string here:
https://github.com/vector-im/element-x-android/blob/41d0d21c808353f91a0d4ceedcb4321a94cf3f31/libraries/featureflag/api/src/main/kotlin/io/element/android/libraries/featureflag/api/FeatureFlags.kt#L32

"Render poll events in the timeline"

since it also a flag to create poll. I propose "Create poll and render poll events in the timeline", but you choose.

bmarty · 2023-08-29T09:37:12Z

...oll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollPresenter.kt

+private const val MIN_ANSWERS = 2;
+private const val MAX_ANSWERS = 20;
+private const val MAX_ANSWER_LENGTH = 240;
+private const val MAX_SELECTIONS = 1;


remove the ;. You are doing too much Rust :)

bmarty · 2023-08-29T09:38:20Z

...oll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollPresenter.kt

+    },
+    restore = {
+        mutableStateOf(
+            when {


There is a problem here, you are not using it:

Suggested change

when {

when(it) {

Oh damn, thanks, this is fatigue, I was wondering all day why intellij was underlying that when and couldn't fathom.

bmarty · 2023-08-29T09:41:19Z

...oll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollPresenter.kt

+    return map { answer ->
+        Answer(
+            text = answer,
+            canDelete = this.size > MIN_ANSWERS,


Maybe confirm with product, but I fill weird not being able to delete an option if there is only 2 options. It's still possible to add an option, then delete the undesired one, so not a big deal though.

This was explicitly specced by Product

bmarty · 2023-08-29T09:46:12Z

...oll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollPresenter.kt

+                            pollKind = pollKind,
+                        )
+                        // analyticsService.capture(PollCreate()) // TODO: Send PollCreate analytics.
+                        navigateUp()


Is there a risk that calling this will cancel the poll creation (since the scope will be cancelled)?

Also we generally let the Node handle the navigation, so better to follow the existing pattern. The presenter set a boolean to the State that the flow is over, and the View communicate to the Node to handle the navigation.

Is there a risk that calling this will cancel the poll creation (since the scope will be cancelled)?

Yes there is, though generally the rust sdk is very quick and I never saw it happen. In general I think this might be a bigger architectural problem as all our presenters use rememberCoroutineScope().

Also we generally let the Node handle the navigation, so better to follow the existing pattern. The presenter set a boolean to the State that the flow is over, and the View communicate to the Node to handle the navigation.

I've seen another case where a custom navigator has been used (MessagesNavigator) so I thought this approach would be better, the added benefit is that it is testable.

bmarty · 2023-08-29T09:47:52Z

...oll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollPresenter.kt

+                        // analyticsService.capture(PollCreate()) // TODO: Send PollCreate analytics.
+                        navigateUp()
+                    } else {
+                        Timber.e("Cannot create poll")


I guess this else block can be removed, the log is not adding lots of value. Or just change it to a debug log.

bmarty · 2023-08-29T09:51:15Z

...res/poll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollView.kt

+
+@OptIn(ExperimentalMaterial3Api::class)
+@Composable
+fun CreatePollView(


I cannot scroll the content when the keyboard is open, So I cannot see the entered text for the option 5 (depending on your phone size).

…feature flag.

julioromano · 2023-08-29T12:25:28Z

Addressed all comments except for: #1143 (comment)

bmarty · 2023-08-29T13:06:27Z

...oll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollPresenter.kt

+                        event.text.substring(0, MAX_ANSWER_LENGTH)
+                    } else {
+                        event.text
+                    }


You can simplify with val text = event.text.take(MAX_ANSWER_LENGTH)

bmarty · 2023-08-29T13:08:57Z

...res/poll/impl/src/main/kotlin/io/element/android/features/poll/impl/create/CreatePollNode.kt

+    @Assisted buildContext: BuildContext,
+    @Assisted plugins: List<Plugin>,
+    presenterFactory: CreatePollPresenter.Factory,
+    analyticsService: AnalyticsService,


For the CI to be happy, this line has to be commented out. We hunt all the warnings.

julioromano · 2023-08-29T13:43:35Z

I've completely commented out the analytics code because the analytics PR is stuck.
I also took care of disabling replies and forwards for poll events.

sonarcloud · 2023-08-29T14:36:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

0.0% Coverage
0.0% Duplication

ElementBot · 2023-08-29T14:36:23Z

...n/kotlin/io/element/android/features/messages/impl/messagecomposer/AttachmentsBottomSheet.kt

@@ -24,6 +24,7 @@
 import androidx.compose.material.ListItem


⚠️ Using a material import while also using the material3 library

bmarty

Thanks for the update! LGTM.

julioromano self-assigned this Aug 24, 2023

julioromano force-pushed the julioromano/create_poll_ui branch from e3c4371 to 222873f Compare August 24, 2023 11:59

julioromano force-pushed the julioromano/create_poll_ui branch 18 times, most recently from 67b23fc to 051ef60 Compare August 25, 2023 11:59

julioromano force-pushed the julioromano/create_poll_ui branch 8 times, most recently from 2c31714 to 319e9c2 Compare August 28, 2023 14:40

julioromano marked this pull request as ready for review August 29, 2023 08:39

julioromano requested a review from a team as a code owner August 29, 2023 08:39

julioromano requested review from bmarty and removed request for a team August 29, 2023 08:39

bmarty requested changes Aug 29, 2023

View reviewed changes

julioromano and others added 10 commits August 29, 2023 14:25

Adds "Poll" entry point in AttachmentsBottomSheet gated by the polls …

27743d5

…feature flag.

CreatePollView

1d2050b

Update screenshots

1f2846e

Truncate answers longer than MAX

b010d03

Localazy all the strings

5648942

First test

7db812e

Back nav confirmation

70612fb

Complete tests

f035167

Fix analytics dependency

2679ce9

Address review comments.

35416d8

julioromano force-pushed the julioromano/create_poll_ui branch from 1418877 to 35416d8 Compare August 29, 2023 12:25

ElementBot and others added 2 commits August 29, 2023 12:33

Update screenshots

74b88dc

break long lines

20ab0aa

bmarty reviewed Aug 29, 2023

View reviewed changes

julioromano added 3 commits August 29, 2023 15:12

Custom action list for poll message bubbles

ef2a8bf

Comment out all analytics code and add todo.

4efd34c

simplify statement

6357521

Add test

10d07d1

ElementBot reviewed Aug 29, 2023

View reviewed changes

bmarty approved these changes Aug 29, 2023

View reviewed changes

julioromano merged commit 633d528 into develop Aug 29, 2023
16 of 18 checks passed

julioromano deleted the julioromano/create_poll_ui branch August 29, 2023 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"Create poll" UI #1143

"Create poll" UI #1143

julioromano commented Aug 24, 2023 •

edited

github-actions bot commented Aug 24, 2023 •

edited

codecov bot commented Aug 25, 2023 •

edited

bmarty left a comment •

edited

bmarty Aug 29, 2023

bmarty Aug 29, 2023

bmarty Aug 29, 2023

julioromano Aug 29, 2023

bmarty Aug 29, 2023

julioromano Aug 29, 2023

bmarty Aug 29, 2023

julioromano Aug 29, 2023 •

edited

bmarty Aug 29, 2023 •

edited

bmarty Aug 29, 2023 •

edited

julioromano commented Aug 29, 2023

bmarty Aug 29, 2023

bmarty Aug 29, 2023

julioromano commented Aug 29, 2023

sonarcloud bot commented Aug 29, 2023

ElementBot Aug 29, 2023

bmarty left a comment

"Create poll" UI #1143

"Create poll" UI #1143

Conversation

julioromano commented Aug 24, 2023 • edited

github-actions bot commented Aug 24, 2023 • edited

codecov bot commented Aug 25, 2023 • edited

Codecov Report

bmarty left a comment • edited

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

julioromano Aug 29, 2023 • edited

Choose a reason for hiding this comment

bmarty Aug 29, 2023 • edited

Choose a reason for hiding this comment

bmarty Aug 29, 2023 • edited

Choose a reason for hiding this comment

julioromano commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julioromano commented Aug 29, 2023

sonarcloud bot commented Aug 29, 2023

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

julioromano commented Aug 24, 2023 •

edited

github-actions bot commented Aug 24, 2023 •

edited

codecov bot commented Aug 25, 2023 •

edited

bmarty left a comment •

edited

julioromano Aug 29, 2023 •

edited

bmarty Aug 29, 2023 •

edited

bmarty Aug 29, 2023 •

edited