Skip to content
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

Show who voted for what in poll results #28305

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

timvahlbrock
Copy link

@timvahlbrock timvahlbrock commented Oct 26, 2024

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation. not applicable
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

Closes #28488

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Oct 26, 2024
@timvahlbrock timvahlbrock marked this pull request as ready for review November 1, 2024 12:53
@timvahlbrock timvahlbrock requested a review from a team as a code owner November 1, 2024 12:53
@timvahlbrock
Copy link
Author

I tried to create tests as well for whether the face pile on the poll options is hidden when the room has more than five members, but I found it very hard to do without relying much on implementation detail.

@timvahlbrock
Copy link
Author

I'm also open for other approaches to opening the detailed result list. One idea might be to make a separate, underlined span that just says "Open full results" next to it.

@timvahlbrock
Copy link
Author

Strict mode commit was temporarily reverted because of #28306

@timvahlbrock
Copy link
Author

Also found the time to add an end to end test for the vote results dialog. If there is anything else that I can do to progress this PR further, please let me know.

@dbkr
Copy link
Member

dbkr commented Nov 18, 2024

I see this has been sent to design & product for review, but I'm not really sure what this PR is trying to do. A description, linked bug and screenshot would help.

That said, I suspect the answer from product will be that polls need to stay consistent between clients, so if this makes web display more than Element X, we'd also need to make corresponding changes there and land them around the same time.

@dbkr dbkr removed their request for review November 18, 2024 15:28
@timvahlbrock
Copy link
Author

I see this has been sent to design & product for review, but I'm not really sure what this PR is trying to do. A description, linked bug and screenshot would help.

Oh sure. Damn I wanted to do that but forgot it again. Will do.

That said, I suspect the answer from product will be that polls need to stay consistent between clients, so if this makes web display more than Element X, we'd also need to make corresponding changes there and land them around the same time.

Okay. I will take a look if I can manage to do that.

@timvahlbrock timvahlbrock changed the title Show detailed poll results Show who voted for what in poll results Nov 18, 2024
@timvahlbrock
Copy link
Author

Screenshots from the End to End Tests:

Before:

image

After:

image image

@dbkr
Copy link
Member

dbkr commented Nov 19, 2024

Thanks, so this isn't actually as big a change as I'd guessed. Maybe this would be fine to land without a corresponding Element X change.

@timvahlbrock
Copy link
Author

Thanks, so this isn't actually as big a change as I'd guessed. Maybe this would be fine to land without a corresponding Element X change.

Yeah, if not, maybe it would be sufficient to show a text on element x like "Users on other platforms may see what you voted for", while the feature hasn't been implemented yet.

@@ -0,0 +1,82 @@
/*
Copyright 2024 New Vector Ltd.
Copyright 2021, 2022 The Matrix.org Foundation C.I.C.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wrote this file, the copyright is yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show who voted what in poll results
3 participants