Support poll vote/unvote #939

PIG208 · 2024-09-12T19:11:15Z

~~This is stacked on top of #912.~~

Fixes: #166

PIG208 · 2024-10-07T23:59:40Z

Preview

chrisbobbe

Thanks! Comments below.

Also, I noticed that the web app has a "pressed" style for the button. Do we want to either do that here or make an issue and TODO for it? Like with reactions, I think it helps the user know that the UI has responded to their gesture, in case it takes a while for the event to come through.

I guess my cursor isn't captured in this screenshot :) but you can imagine it there:

chrisbobbe · 2024-10-08T19:59:18Z

lib/api/route/submessage.dart

+    // There is also 'zform' for trivia quiz bot, but we currently do not have
+    // plan to support that.


nit:

'do not have a plan' or

'do not have plans' or

'do not plan'

Hmm, somehow I perfectly missed all the correct options! 😓

chrisbobbe · 2024-10-08T20:02:20Z

lib/api/route/submessage.dart

+    // There is also 'zform' for trivia quiz bot, but we currently do not have
+    // plan to support that.
+    'msg_type': RawParameter('widget'),
+    'content': RawParameter(jsonEncode(content)),


Would we get the same result from 'content': content as we do from this, and if so, would it be preferable because it looks simpler?

The case of content is a bit peculiar, because the API expects a plain string instead of an object, while the string is expected to be some JSON encoded value.

I think it would be good to mention this in comments.

Is there an actual difference in behavior here from 'content': content?

Remember that parameters on a Zulip API endpoint as a whole are encoded as HTML form data, not as JSON. This is a longstanding gap in our API docs; most recent discussion here:
https://chat.zulip.org/#narrow/channel/138-user-questions/topic/.E2.9C.94.20Slack.20webhook.20incompatible.3F/near/1923677
with link to a previous discussion.

So there's no enclosing JSON object with 'content' as a key, which could then have either a string or an object as a value — there's just a string-to-string key-value map, with content as a key and some string as a value. If that value is itself the JSON-encoding of some object, then that's just the normal way an object appears as a parameter in the Zulip API.

I did a quick check and this passes the newly-added test:

Suggested change

'content': RawParameter(jsonEncode(content)),

'content': content,

chrisbobbe · 2024-10-08T20:05:04Z

test/model/message_test.dart

-              options: (options != null)
-                ? options.map((e) => e.text).toList()
+              options: (optionVoterIdsPairs != null)
+                ? optionVoterIdsPairs.map((e) => e.$1).toList()


Oh, odd, I guess $1 refers to the first element of the tuple? So it's not zero-indexed.

Yeah, records do not have the prettiest syntax.

chrisbobbe · 2024-10-08T20:18:08Z

lib/widgets/poll.dart

-          children: [
-            ConstrainedBox(
-              constraints: const BoxConstraints(minWidth: 25).tighten(height: 25),
+      return Row(


poll [nfc]: Move vote count paddings to a box

This does not visually change the paddings.

This doesn't seem to be true. Here's before/after this change, with message https://chat.zulip.org#narrow/stream/7-test-here/topic/new.20poll.20test/near/1956979 :

Before After

When I open the two images in different browser tabs and switch between them, I see a layout change. Do you reproduce?

Hmm, I can see that in the screenshots, but not on my Android device.

When the bottom: 5 padding wraps the whole row (before this commit), instead of just the vote-count rectangle, there will be 5px whitespace below the text (option name + voters' names) even when the text takes more vertical space than the vote-count rectangle.

I think you can reproduce if you make an option with a name so long that it takes two or three lines on your device.

This might be a desired change, but it's not an NFC one :)

chrisbobbe · 2024-10-08T20:27:45Z

lib/widgets/poll.dart

+                // This is only in effect
+                // when the vote count has more than 2 digits.


I'm a little suspicious of a comment like this :) because I don't know if it's true as written for all values of the user's system bold-text and text-size settings. If this needs a comment, maybe it could be something like:

// Inner padding preserves whitespace even when the text's // width approaches the button's min-width (e.g. because // there are three digits).

alya · 2024-10-08T21:11:16Z

Screenshots and CZO discussion look good to me!

This would also apply to emoji reaction pills. This would be an UX improvement to let the user know that the data is going to be updated. See also: zulip#939 (review) Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2024-10-09T18:58:22Z

The "pressed" UX might fit better in a separate PR. I guess we need to have a way to mark some data as "dirty" until an event comes and update it (but there could also be cases where we expect multiple events).

I also noticed that my previous screenshots have a non-default sized text. The 22px font size for the vote count text is actually inadequate. Updated that to 20px with new screenshots:

Updated screenshots

chrisbobbe · 2024-10-09T22:41:29Z

The "pressed" UX might fit better in a separate PR. I guess we need to have a way to mark some data as "dirty" until an event comes and update it (but there could also be cases where we expect multiple events).

I was just thinking about highlighting the button when the user's finger is on it; I wasn't thinking about the state between the tap and the event arriving.

—Ah, I guess, what does the web app do? I was assuming it was a simple "pressed" state, but looking closer, I think it might be the more complicated thing where it highlights the button between the tap and the event arrival.

gnprice · 2024-10-10T00:28:59Z

I think it might be the more complicated thing where it highlights the button between the tap and the event arrival.

That more complicated thing sounds like when it has an effect, it'd be liable to make it feel like the button was stuck. (Though I haven't tried to play with this aspect of web's UX.)

I'd be glad to have immediate feedback while the user's finger is there, and perhaps animating back to the resting state over a short fixed duration thereafter like 100ms, while not trying to have any longer-lived "dirty" state. This would make it similar to the existing behavior on the mark-as-read button.

chrisbobbe · 2024-10-10T21:04:07Z

That more complicated thing sounds like when it has an effect, it'd be liable to make it feel like the button was stuck. (Though I haven't tried to play with this aspect of web's UX.)

I agree; let's not do this. I think Vlad likes a scale animation for the "pressed" state, like we did for the mark-as-read button in 791b703.

Then I think #939 (comment) is still pending, about a layout-change commit that should be unmarked NFC. One consequence of the change is that it removes any spacing between two options when the first one's text runs onto multiple lines:

Before	After

I don't love that consequence, but it's helped in a later commit that provides 2.5px spacing there (from the verticalPadding). One possible improvement might be to bump that up to 5px by putting a 2.5px margin under the text.

chrisbobbe · 2024-10-10T21:09:32Z

Then also, as discussed in the office just now: in the place where we use a fixed height, we'll want a min-height instead, so that it doesn't break with a large system text size setting:

Current revision	Changed to min-height

This would also apply to emoji reaction pills. This would be an UX improvement to let the user know that the data is going to be updated. See also: zulip#939 (review) Signed-off-by: Zixuan James Li <[email protected]>

Visually, this does not change the appearence of the vote count box. This does not implement local echoing for voting. Instead, we rely on the submessage events to get the updates after voting. For accessbility, the touch target is larger than the button. See also: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Poll.20vote.2Funvote.20UI/near/1952724 We add a TODO for pending poll votes visual indicator. This would also apply to emoji reaction pills. See also: zulip#939 (review) Fixes: zulip#166 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2024-10-22T19:27:43Z

This PR has been updated. Thanks for the review!

gnprice

Thanks for the revision! Generally this looks good; one question below.

Since this revision changed the layout, please also post updated screenshots.

gnprice · 2024-10-25T03:42:09Z

lib/widgets/poll.dart

-      return Padding(
-        padding: const EdgeInsets.only(bottom: 5),


poll: Move vote count paddings inside children of Row

It looks like in this revision, this commit actually moves the padding only inside the first child, and drops the padding from the second one.

This preserves most of the visuals. Except that when the vote count box is shorter than the voter names text, the padding between rows is gone.

That seems undesirable. What if we include padding on both children, like in the previous revision?

I think this change in this revision may have been in response to this thread:
#939 (comment)
but I don't really understand the motivation for removing the padding from the voter names.

Thanks! Pulled that back and edited the comment. I removed it previously because I thought it was somewhat a hacky workaround, which could be confusing.

Cool. Yeah, I see it as putting the same padding on each sibling — which seems like a normal reasonable thing to do, not anything hacky.

Visually, this does not change the appearence of the vote count box. This does not implement local echoing for voting. Instead, we rely on the submessage events to get the updates after voting. For accessbility, the touch target is larger than the button. See also: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Poll.20vote.2Funvote.20UI/near/1952724 We add a TODO for pending poll votes visual indicator. This would also apply to emoji reaction pills. See also: zulip#939 (review) Fixes: zulip#166 Signed-off-by: Zixuan James Li <[email protected]>

gnprice

Great, thanks for changing that back.

Small comments below — most of them about a follow-up in that same area.

gnprice · 2024-10-26T22:53:54Z

lib/widgets/poll.dart

-      return Padding(
-        padding: const EdgeInsets.only(bottom: 5),


Cool. Yeah, I see it as putting the same padding on each sibling — which seems like a normal reasonable thing to do, not anything hacky.

gnprice · 2024-10-26T22:55:47Z

lib/widgets/poll.dart

+            child: Padding(
+              // This padding and the bottom padding on the vote count box
+              // extends the row by the same extent. This ensures that there
+              // still will be consistent spacing between rows, when the text
+              // takes more vertical space than the vote count box.
+              padding: const EdgeInsets.only(bottom: verticalPadding),


Why is this padded only on the bottom, while the buttons are padded top and bottom?

gnprice · 2024-10-26T22:57:36Z

lib/widgets/poll.dart

+        Padding(
+          // `verticalPadding` out of 6 logical pixels come from the first
+          // option row.
+          padding: const EdgeInsets.only(bottom: 6 - verticalPadding),


This reasoning assumes that the first option row has some top padding. But in this version that only holds if the button is taller than the text, right?

Ah, I guess if the text is long, then there's still top padding because CrossAxisAlignment.baseline means the option text starts at the same level as the count and expands down from there, and so the button still extends further upward from the baseline than the text does.

Still: the much simpler way for the reader to reach the conclusion that there's always top padding on an option row would be if both children of the row had the same top padding.

Ah, I guess if the text is long, then there's still top padding because CrossAxisAlignment.baseline means the option text starts at the same level as the count and expands down from there, and so the button still extends further upward from the baseline than the text does.

Yeah, exactly. I've made them symmetric in the new revision.

gnprice · 2024-10-26T22:58:45Z

lib/widgets/poll.dart

+          Padding(
+            // This is consistent with the option items' top padding.
+            padding: const EdgeInsets.only(top: verticalPadding),


Similarly here.

I think this probably all becomes cleaner to think about if the text gains verticalPadding of top padding.

gnprice · 2024-10-26T23:35:44Z

lib/api/route/submessage.dart

+/// https://zulip.readthedocs.io/en/latest/subsystems/widgets.html#polls-todo-lists-and-games
+Future<void> sendSubmessage(ApiConnection connection, {


This dartdoc should also link to the zulip-mobile implementation, given that that's one of the sources we relied on in writing this implementation (as the commit message says).

The route is not documented in the Zulip API Documentation. Instead, we refer to the zulip-mobile implementation and https://zulip.readthedocs.io/en/latest/subsystems/widgets.html#polls-todo-lists-and-games. Signed-off-by: Zixuan James Li <[email protected]>

The `preparePollMessage` helper was only using `PollOption` for convenience. It doesn't actually need the `PollOption` instances. This allows us to make changes to `PollOption` more freely. Signed-off-by: Zixuan James Li <[email protected]>

This allows us to lookup the key that corresponds to the PollOption without referencing back the map. Signed-off-by: Zixuan James Li <[email protected]>

This makes it easier to follow font sizes of UI elements in this widget. We leave textStyleVoterNames as-is because we never need to override the font size. Signed-off-by: Zixuan James Li <[email protected]>

On devices with non-default text scaling, the fixed size vote count box might be too short to fit in the text. We should only set the min height to fix that. Signed-off-by: Zixuan James Li <[email protected]>

We already center the text with `Center`, which is preferred because it also centers vertically. Signed-off-by: Zixuan James Li <[email protected]>

Visually, this does not change the appearence of the vote count box. This does not implement local echoing for voting. Instead, we rely on the submessage events to get the updates after voting. For accessbility, the touch target is larger than the button. See also: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Poll.20vote.2Funvote.20UI/near/1952724 We add a TODO for pending poll votes visual indicator. This would also apply to emoji reaction pills. See also: zulip#939 (review) Fixes: zulip#166 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2024-10-28T23:05:55Z

Pushed a new revision. Thanks for the review!

Now we use vertically symmetric padding for all the option items, and limit the one-sided paddings.
This also fixed an issue with the "This poll has no options yet" text where the bottom padding is missing.
I added 5 - verticalPadding to the entire poll widget so that there is still 5px space after it.

before	after

gnprice

Thanks for the revision! Glad to have that layout issue fixed — and I think the layout logic all makes sense to me now. Just a few style comments below.

gnprice · 2024-10-29T06:05:03Z

lib/widgets/poll.dart

+            Padding(
+              // This is consistent with the option rows' padding.


This change makes this commit no longer NFC, right?
ceca4ce poll [nfc]: Move vote count paddings inside children of Row

It's also not part of what that summary describes. So I think it should just become a separate commit, fixing that layout bug you noted.

gnprice · 2024-10-29T06:07:27Z

lib/widgets/poll.dart

+          Padding(
+            // `verticalPadding` out of 6 logical pixels
+            // come from the first option row.
+            padding: const EdgeInsets.only(bottom: 6 - verticalPadding),
+            child: question),


nit: this is a bit simpler to express with a SizedBox spacer:

Suggested change

Padding(

// `verticalPadding` out of 6 logical pixels

// come from the first option row.

padding: const EdgeInsets.only(bottom: 6 - verticalPadding),

child: question),

question,

// `verticalPadding` out of 6px come from the first option row.

const SizedBox(height: 6 - verticalPadding),

gnprice · 2024-10-29T06:08:21Z

lib/widgets/poll.dart

+    return Padding(
+      // `verticalPadding` out of 5 logical pixels
+      // come from the last option row.
+      padding: const EdgeInsets.only(bottom: 5 - verticalPadding),


Then similarly this padding can be expressed as a spacer at the end of the column's children. I think that makes it significantly clearer what's going on, because it keeps things in a consistent order.

It makes it easier to adjust the size of the vote count box touch target later. This preserves the visuals. In particular, we add bottom paddings to both the vote count box and the option/voters text to ensure that there is always 5px between option rows, as it was before. The end padding is equivalent to the spacing previously specified on the Row; the bottom padding is equivalent to the padding specified on the outer (now removed) Padding. The ConstrainedBox is expanded because it now also contains the paddings. Signed-off-by: Zixuan James Li <[email protected]>

Previously, the 5px bottom padding only existed when there were option rows. This padding should be applied to the placeholder text as well. Signed-off-by: Zixuan James Li <[email protected]>

Visually, this does not change the appearence of the vote count box. This does not implement local echoing for voting. Instead, we rely on the submessage events to get the updates after voting. For accessbility, the touch target is larger than the button. See also: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Poll.20vote.2Funvote.20UI/near/1952724 We add a TODO for pending poll votes visual indicator. This would also apply to emoji reaction pills. See also: zulip#939 (review) Fixes: zulip#166 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2024-10-29T17:20:56Z

Thanks! The PR has been updated.

Previously, there were only bottom and end paddings; we redistribute the vertical paddings so the vote count box is padded on both sides. The end padding remains 5px because we want to keep the options aligned to the start with the surrounding messages. This change also affects the poll question, where 2.5px of space gets moved to the first option row. Similarly, the end of the poll was previously padded by the last poll option alone. Now 2.5px of space gets moved to the `Padding` wrapping the entire `Column` of the poll. Signed-off-by: Zixuan James Li <[email protected]>

Visually, this does not change the appearence of the vote count box. This does not implement local echoing for voting. Instead, we rely on the submessage events to get the updates after voting. For accessbility, the touch target is larger than the button. See also: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Poll.20vote.2Funvote.20UI/near/1952724 We add a TODO for pending poll votes visual indicator. This would also apply to emoji reaction pills. See also: zulip#939 (review) Fixes: zulip#166 Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-11-01T23:33:00Z

Thanks! Looks good now — merging, with two tweaks:

-    poll: Enlarge and vetically center vote count box
+    poll: Enlarge and vertically center vote count box


-+        // `verticalPadding` out of 6px come from the first option row.
++        // `verticalPadding` out of 6px comes from the first option row.

(I know the latter comes from my suggestion above; rereading, I'd incompletely edited it from the previous version.)

PIG208 force-pushed the pr-poll-vote branch 2 times, most recently from 05d75c4 to 30f4ae1 Compare October 3, 2024 21:06

PIG208 marked this pull request as ready for review October 3, 2024 21:07

PIG208 force-pushed the pr-poll-vote branch 2 times, most recently from 1d47050 to ec71e83 Compare October 3, 2024 21:08

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Oct 3, 2024

PIG208 assigned chrisbobbe Oct 3, 2024

PIG208 requested a review from chrisbobbe October 3, 2024 21:08

PIG208 changed the title ~~WIP Support poll vote/unvote~~ Support poll vote/unvote Oct 3, 2024

This comment was marked as outdated.

Sign in to view

PIG208 removed the maintainer review PR ready for review by Zulip maintainers label Oct 3, 2024

PIG208 force-pushed the pr-poll-vote branch 2 times, most recently from 6de28fa to b0a4dc5 Compare October 7, 2024 23:58

PIG208 requested a review from alya October 8, 2024 00:00

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Oct 8, 2024

chrisbobbe reviewed Oct 8, 2024

View reviewed changes

PIG208 force-pushed the pr-poll-vote branch from b0a4dc5 to 67765ff Compare October 9, 2024 18:47

PIG208 force-pushed the pr-poll-vote branch from 67765ff to 7c63197 Compare October 9, 2024 18:53

PIG208 requested a review from chrisbobbe October 9, 2024 18:58

PIG208 force-pushed the pr-poll-vote branch from 4e1b6d5 to a7001fe Compare October 22, 2024 18:19

gnprice reviewed Oct 25, 2024

View reviewed changes

PIG208 force-pushed the pr-poll-vote branch from a7001fe to 18c24d3 Compare October 25, 2024 15:50

gnprice reviewed Oct 27, 2024

View reviewed changes

PIG208 added 7 commits October 28, 2024 17:50

poll [nfc]: Store key on PollOption

5e9f0a3

This allows us to lookup the key that corresponds to the PollOption without referencing back the map. Signed-off-by: Zixuan James Li <[email protected]>

poll [nfc]: Override font size at where the TextStyle is used

1c8406f

This makes it easier to follow font sizes of UI elements in this widget. We leave textStyleVoterNames as-is because we never need to override the font size. Signed-off-by: Zixuan James Li <[email protected]>

poll [nfc]: Set vote count box's minHeight with ConstrainedBox

72f09c1

On devices with non-default text scaling, the fixed size vote count box might be too short to fit in the text. We should only set the min height to fix that. Signed-off-by: Zixuan James Li <[email protected]>

poll [nfc]: Remove redundant TextAlign.center

8e05574

We already center the text with `Center`, which is preferred because it also centers vertically. Signed-off-by: Zixuan James Li <[email protected]>

poll [nfc]: Explain vote count box's inner padding

255aed0

PIG208 force-pushed the pr-poll-vote branch from 18c24d3 to b4a96c5 Compare October 28, 2024 22:55

gnprice reviewed Oct 29, 2024

View reviewed changes

PIG208 added 2 commits October 29, 2024 13:03

poll: Add missing padding for options placeholder text

0a53ade

Previously, the 5px bottom padding only existed when there were option rows. This padding should be applied to the placeholder text as well. Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the pr-poll-vote branch from b4a96c5 to 8982bea Compare October 29, 2024 17:17

PIG208 added 2 commits November 1, 2024 16:30

gnprice force-pushed the pr-poll-vote branch from 8982bea to 9fde534 Compare November 1, 2024 23:33

gnprice merged commit 9fde534 into zulip:main Nov 1, 2024

		// There is also 'zform' for trivia quiz bot, but we currently do not have
		// plan to support that.

	'content': RawParameter(jsonEncode(content)),
	'content': content,

		// This is only in effect
		// when the vote count has more than 2 digits.

		/// https://zulip.readthedocs.io/en/latest/subsystems/widgets.html#polls-todo-lists-and-games
		Future<void> sendSubmessage(ApiConnection connection, {

		Padding(
		// This is consistent with the option rows' padding.

Support poll vote/unvote #939

Support poll vote/unvote #939

Conversation

PIG208 commented Sep 12, 2024 • edited Loading

This comment was marked as outdated.

PIG208 commented Oct 7, 2024 • edited Loading

chrisbobbe left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alya commented Oct 8, 2024

PIG208 commented Oct 9, 2024

chrisbobbe commented Oct 9, 2024 • edited Loading

gnprice commented Oct 10, 2024

chrisbobbe commented Oct 10, 2024 • edited Loading

chrisbobbe commented Oct 10, 2024

PIG208 commented Oct 22, 2024

gnprice 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

gnprice 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

PIG208 commented Oct 28, 2024

gnprice 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

PIG208 commented Oct 29, 2024

gnprice commented Nov 1, 2024

PIG208 commented Sep 12, 2024 •

edited

Loading

PIG208 commented Oct 7, 2024 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe Oct 9, 2024 •

edited

Loading

chrisbobbe commented Oct 9, 2024 •

edited

Loading

chrisbobbe commented Oct 10, 2024 •

edited

Loading