-
Notifications
You must be signed in to change notification settings - Fork 782
Edit Polls and Allow Undisclosed Polls #5020
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
Conversation
Matrix SDKIntegration Tests Results:
|
* develop: (66 commits) toolbar management (#4887) adding changelog entry adding back periodic flag when scheduling automatic background workers Fix enum class warning Split long lines Done by matrix-org/matrix-analytics-events#16 Add new class in analytics plan Fix conditional for Delight issue automation Add missing import in kdoc Update kdoc Enable Delight issue automation Fix an error in string resource (#4997) Changelog Add some unit test for the command parser. Not all commands are covered, could add more tests later. data class. use sealed interface Small cleanup Command parser is not a static object anymore Add changelog Use Throwable.isLimitExceededError extension Do not automatically retry 429 with a too long delay ...
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.
Very nice work, thanks!
Just a few small remarks, but nothing blocking from me
data class PollUndisclosed(override val optionId: String, | ||
override val optionAnswer: String, | ||
val isSelected: Boolean | ||
) : PollOptionViewState(optionId, optionAnswer) |
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.
Happy to see that the arch here is nice to do such addition of a new state!
@@ -51,4 +51,12 @@ sealed class PollOptionViewState(open val optionId: String, | |||
val votePercentage: Double, | |||
val isWinner: Boolean | |||
) : PollOptionViewState(optionId, optionAnswer) | |||
|
|||
/** | |||
* Represent a poll that is closed, votes will be hidden until the poll is ended. |
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 replace closed
by undisclosed
to avoid confusion between close
and ended
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.
Right, done.
_viewEvents.post(CreatePollViewEvents.Success) | ||
} | ||
} | ||
} | ||
|
||
private fun sendEditedPoll(editedEventId: String, pollType: PollType, question: String, options: List<String>) { | ||
val editedEvent = room.getTimeLineEvent(editedEventId) ?: return | ||
room.editPoll(editedEvent, pollType, question, options) |
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.
Nit: room.editPoll()
first parameter could be an eventId: String
to let the SDK user have a nicer API.
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.
But we don't have the room
instance in EventEditor
. Do we really want to inject session
and pass the roomId
and eventId
?
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.
OK, the API is similar to the other ones, you can leave it like this, sorry.
@@ -53,14 +53,8 @@ | |||
|
|||
<im.vector.app.core.preference.VectorSwitchPreference | |||
android:defaultValue="false" | |||
android:key="SETTINGS_LABS_ENABLE_POLLS" | |||
android:title="@string/labs_enable_polls" /> |
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.
Can you add a comment above the string resource labs_enable_polls
like <!-- TODO Remove -->
please?
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.
Done, I was thinking to ask you about this.
Quick smoke test, everything worked fine. |
Fixes: #5036, Fixes #5037
Figma