Poll Feature - Create #4360

onurays · 2021-10-28T07:53:48Z

* develop: (129 commits) Improve Rx sequence regarding listener adding changelog entry using correct license for matrix-sdk test extending the room name resolved to create a dedicated room name data class which contains a normalized version of the room name Remove shortcut as soon as a PIN code is set ensuring the store migration class is always equal to other store migration instances - is needed as realm will throw if multiple migration instances are created and they don't match removing extra query definition by chaining the query creation with modifiers removing noisy log which duplicates a type clause and fixing when casing formatting to have a case per line documenting the different query cases making the isNormalized function an extension and internal to the sdk Cache immutable value Do not show shortcuts if a PIN code is set Remove (disable) shortcut if a room is left Ensure ShortcutsHandler get all the joined rooms #4168 Add `sortOrder: RoomSortOrder` parameter, with no API break Clean code adding normalised room display name field and making use of it when filtering rooms by name - fixes non latin-1 character set room names from being ignored when searching with inexact casing adding normalisation to the query string cases making value processing an injectable class, it will need to have its own dependencies to support normalisation porting QueryStringValue to sealed interface with a sub category for the content based values - allows for handling those cases separately for normalisation ... # Conflicts: # vector/src/main/java/im/vector/app/core/di/ScreenComponent.kt

github-actions · 2021-10-28T15:29:16Z

Unit Test Results

  62 files ±0   62 suites ±0 1m 4s ⏱️ -1s
118 tests ±0 118 ✔️ ±0 0 💤 ±0 0 ❌ ±0
350 runs ±0 350 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 299d81f. ± Comparison against base commit 868548d.

♻️ This comment has been updated with latest results.

bmarty

Nice work, thanks.
Some remarks plus:

How to the attachment picker is displayed if the keyboard is opened?
Maybe temporarily hide the option to create poll in this PR so that we can merge it once it's approved

vector/src/main/java/im/vector/app/features/createpoll/CreatePollViewModel.kt

vector/src/main/java/im/vector/app/features/createpoll/CreatePollAction.kt

vector/src/main/java/im/vector/app/features/createpoll/CreatePollActivity.kt

vector/src/main/java/im/vector/app/features/createpoll/CreatePollViewModel.kt

bmarty · 2021-10-28T15:47:38Z

Also please have a look at the CI

bmarty · 2021-10-28T15:59:29Z

Is there an existing issue? If yes it should be linked to this PR
Also is there a Figma link?

ganfra

Some remarks

vector/src/main/java/im/vector/app/features/createpoll/CreatePollActivity.kt

vector/src/main/java/im/vector/app/features/createpoll/CreatePollFragment.kt

onurays · 2021-10-29T14:21:27Z

Nice work, thanks. Some remarks plus:

How to the attachment picker is displayed if the keyboard is opened?

Maybe temporarily hide the option to create poll in this PR so that we can merge it once it's approved

It is hiding the text composer. Composer design is changed completely. I will also implement it next week. So this shouldn't block this PR.
Done, good idea.

bmarty

Thanks for the update.
A few more comments for you. Also can you please check the CI status?

bmarty · 2021-11-02T12:32:34Z

vector/src/main/java/im/vector/app/features/navigation/DefaultNavigator.kt

-        val intent = CreatePollActivity.getIntent(context, CreatePollArgs(roomId = roomId))
+        val intent = CreatePollActivity.getIntent(
+                context,
+                CreatePollArgs(roomId = roomId, minOptionsCount = CreatePollViewModel.MIN_OPTIONS_COUNT)


What is the point to pass minOptionsCount as an Activity parameter?

I thought it was necessary to use it in CreatePollViewState. Fixed by using const value in ViewModel directly.

bmarty · 2021-11-02T12:34:13Z

vector/src/main/java/im/vector/app/features/poll/create/CreatePollViewModel.kt

+                        canCreatePoll = canCreatePoll(question, options),
+                        canAddMoreOptions = options.size < MAX_OPTIONS_COUNT
+                )
+            }


bmarty · 2021-11-02T12:35:58Z

vector/src/main/java/im/vector/app/features/poll/create/CreatePollViewState.kt

 ) : MavericksState {

    constructor(args: CreatePollArgs) : this(
-            roomId = args.roomId
+            roomId = args.roomId,
+            options = List(args.minOptionsCount) { "" }


This is not the best place to init options. I would have done it on line 24 instead

gaelledel · 2021-11-02T14:56:55Z

@onurays I'm not able to see the polls feature in the test build you sent

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

gaelledel

We need to left align the close button with the content below please ( cross aligned with title under)
Change the colour of the text field frames so it matches Figma colours --> Quinary
The option text fields should not grow (this behaviour should only happen for the poll question)
The delete Option button should be centre aligned with the text field
The Create Poll button disabled state visual does not match Figma's disabled state. For more info on buttons and core components please check Compound UI kit.
Weird animation when no options fields are displayed and you press on Add option. The add option text animates downwards. Could you remove this?
On user input in the text field: the Return key Dismiss the keyboard and the field should return to unfocussed state.
Enlarge the default height of the question to double size. The field should only grow if the character limit is reached. The return button should dismiss the keyboard and the field should be unfocussed. See Figma here https://www.figma.com/file/tepA3ELWBE5v4O2WB8PqWn/Polls?node-id=496%3A52205
The Create poll CTA should not move above the composer. It should remain under the Add option button at all times

gaelledel

Thanks for the edits @onurays!

ouchadam · 2021-11-10T15:25:27Z

how does the screen look in dark mode?

ouchadam

small remark about double checking dark mode, otherwise looks good!

bmarty

Just a few more questions / remark.
Also please fix errors reported by the CI

bmarty · 2021-11-10T16:15:26Z

vector/src/main/java/im/vector/app/features/form/FormEditTextWithDeleteItem.kt

+
+        holder.textInputEditText.isEnabled = enabled
+        if (singleLine) {
+            holder.textInputEditText.setSingleLine()


When doing binding, we have to take care of the recycling of views. So a else block is needed here I think

bmarty · 2021-11-10T16:15:47Z

vector/src/main/java/im/vector/app/features/form/FormEditTextWithDeleteItem.kt

+        }
+        imeOptions?.let {
+            holder.textInputEditText.imeOptions = it
+        }


same remark here

bmarty · 2021-11-10T16:17:32Z

vector/src/main/java/im/vector/app/features/poll/create/CreatePollActivity.kt

+
+    override fun onCreate(savedInstanceState: Bundle?) {
+        super.onCreate(savedInstanceState)
+        views.toolbar.visibility = View.GONE


Strange to hide the toolbar here. I guess the toolbar is part of the fragment?

Otherwise, it shows the back button on Toolbar. We are doing the same for CreateDirectRoomActivity and InviteUsersToRoomActivity.

bmarty · 2021-11-10T16:20:01Z

vector/src/main/java/im/vector/app/features/poll/create/CreatePollFragment.kt

+    }
+
+    private val hideToastRunnable = Runnable {
+        views.createPollToast.isVisible = false


I think this can crash if the Fragment has been destroyed

We are using the same mechanism while recording voice messages. It is not crashing because it uses Handler inside. Also, we will never show toast on this screen, since we disable the button.

But I want to keep this to protect against post improvements.

bmarty

The CI is not happy, can you have a look please?

bmarty · 2021-11-15T10:07:13Z

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

+    <string name="create_poll_title">Create Poll</string>
+    <string name="create_poll_question_title">Poll question or topic</string>
+    <string name="create_poll_question_hint">Question or topic</string>
+    <string name="create_poll_options_title">Create options</string>


We discussed about that wording in a Matrix room. Maybe "Options" is better here. What is the status?
CC @gaelledel

@gaelledel wants to use "Create options".

bmarty

Thanks for the update.
I will do a quick change on the style, sth I did not see before, but not blocking.

Onuray Sahin added 9 commits October 25, 2021 14:47

Add poll icon to attachment type selector.

3e03db2

Create poll screen components implemented.

4af4290

Create poll fragment with a title.

cb1d5e8

Create poll UI implementation.

6cee266

Create poll event content.

dd58dd8

Use unstable types.

f9f4317

Remove poll command.

ac299d8

Fix UI issues.

db820ef

onurays requested a review from bmarty October 28, 2021 15:05

bmarty reviewed Oct 28, 2021

View reviewed changes

ganfra reviewed Oct 28, 2021

View reviewed changes

vector/src/main/java/im/vector/app/features/createpoll/CreatePollActivity.kt Outdated Show resolved Hide resolved

vector/src/main/java/im/vector/app/features/createpoll/CreatePollFragment.kt Outdated Show resolved Hide resolved

Onuray Sahin added 2 commits October 29, 2021 16:54

Code review fixes.

40aa017

Limit maximum number of poll options.

19216ae

onurays linked an issue Oct 29, 2021 that may be closed by this pull request

Poll Feature - Create Poll Screen #4367

Closed

4 tasks

onurays requested review from bmarty and ganfra October 29, 2021 14:19

onurays marked this pull request as ready for review November 1, 2021 07:54

onurays changed the title ~~Poll Feature~~ Poll Feature - Create Nov 1, 2021

bmarty reviewed Nov 2, 2021

View reviewed changes

vector/src/main/res/layout/view_attachment_type_selector.xml Show resolved Hide resolved

bmarty and others added 2 commits November 2, 2021 16:34

Make the poll option visible so that it can be tested from the PR

b8ca520

Code review fixes.

cbe07c4

onurays requested review from bmarty and gaelledel November 4, 2021 08:44

gaelledel suggested changes Nov 4, 2021

View reviewed changes

Design review fixes.

a10253c

onurays requested a review from gaelledel November 8, 2021 14:26

Design review fixes.

7546071

gaelledel approved these changes Nov 10, 2021

View reviewed changes

ouchadam approved these changes Nov 10, 2021

View reviewed changes

bmarty reviewed Nov 10, 2021

View reviewed changes

Onuray Sahin added 3 commits November 11, 2021 11:52

Lint fixes.

89db5a6

Code review fixes.

b6717f4

Hide poll icon from attachment type selector for now.

a6c88b7

onurays requested a review from bmarty November 11, 2021 10:42

bmarty requested changes Nov 15, 2021

View reviewed changes

Onuray Sahin added 2 commits November 15, 2021 13:26

Lint fix.

fff47b8

Lint fix.

299d81f

onurays requested a review from bmarty November 15, 2021 12:41

bmarty approved these changes Nov 15, 2021

View reviewed changes

bmarty merged commit 35e2a10 into develop Nov 15, 2021

bmarty deleted the feature/ons/poll branch November 15, 2021 13:05

bmarty mentioned this pull request Nov 15, 2021

Clean up styles #4470

Merged

This was referenced Dec 13, 2021

[Poll] Add a nice poll composer #993

Closed

I can't see total votes, also no secret voting #1942

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Poll Feature - Create #4360

Poll Feature - Create #4360

onurays commented Oct 28, 2021 •

edited

Loading

github-actions bot commented Oct 28, 2021 •

edited

Loading

bmarty left a comment

bmarty commented Oct 28, 2021

bmarty commented Oct 28, 2021

ganfra left a comment

onurays commented Oct 29, 2021

bmarty left a comment

bmarty Nov 2, 2021

onurays Nov 4, 2021

bmarty Nov 2, 2021

bmarty Nov 2, 2021

onurays Nov 4, 2021

gaelledel commented Nov 2, 2021

gaelledel left a comment •

edited by onurays

Loading

gaelledel left a comment

ouchadam commented Nov 10, 2021

ouchadam left a comment

bmarty left a comment

bmarty Nov 10, 2021

onurays Nov 11, 2021

bmarty Nov 10, 2021

onurays Nov 11, 2021

bmarty Nov 10, 2021

onurays Nov 11, 2021

bmarty Nov 10, 2021

onurays Nov 11, 2021

bmarty left a comment

bmarty Nov 15, 2021

onurays Nov 15, 2021

bmarty Nov 15, 2021

bmarty left a comment

Poll Feature - Create #4360

Poll Feature - Create #4360

Conversation

onurays commented Oct 28, 2021 • edited Loading

github-actions bot commented Oct 28, 2021 • edited Loading

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

bmarty commented Oct 28, 2021

bmarty commented Oct 28, 2021

ganfra left a comment

Choose a reason for hiding this comment

onurays commented Oct 29, 2021

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

gaelledel commented Nov 2, 2021

gaelledel left a comment • edited by onurays Loading

Choose a reason for hiding this comment

gaelledel left a comment

Choose a reason for hiding this comment

ouchadam commented Nov 10, 2021

ouchadam left a comment

Choose a reason for hiding this comment

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

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

bmarty left a comment

Choose a reason for hiding this comment

onurays commented Oct 28, 2021 •

edited

Loading

github-actions bot commented Oct 28, 2021 •

edited

Loading

gaelledel left a comment •

edited by onurays

Loading