feat(Poll): add option to randomize poll choices #2082
Conversation
|
Hi @huumn, ready for review. Also: |
|
At first glance this looks pretty solid. Nice work! I'll try to QA and do a more in depth review by early next week. |
|
Ok thanks, would it be ok if I got started on another while I wait @huumn |
|
Yes of course! Have at it. I'll give you triage permissions so you can assign yourself to another issue you like. |
|
Ok great, thanks |
There was a problem hiding this comment.
Welcome to contributing to SN!
I found three issues with your PR, and I am requesting changes because of the first two, but I can see how it's easy to miss them, so no worries.
The first two issues are related to shuffling the options on the render path of the component.
- The options will reshuffle on every render, see video:
2025-04-10.02-20-02.mp4
- Hydration errors on full page load because the HTML rendered on the server will be different:
2025-04-10.02-28-47.mp4
I think both issues can be fixed with useEffect.1
The other issue is a nitpick about code style: I think you're using map to create a new array and shuffle that one since item.poll.options is read-only.
Since I think that's not clear from just reading the code—the 2x map looked unnecessary so I checked why you're doing that—I would recommend to simply create a copy of the options input and then return a shuffled version of that with one sort call, see suggestion. A comment in cases like this where the most obvious solution doesn't work is also nice.
Footnotes
-
Maybe we even want to use
useLayoutEffectto not show the sorted options in the first render but that would be a nitpick and I wouldn't request changes for it—I would just change it myself if I think it will improve UX, so don't worry about this detail too much. ↩
brymut
force-pushed
the
feat/add-random-poll-order
branch
2 times, most recently
from
28ba4fd to
468595d
Compare
There was a problem hiding this comment.
The hydration error is gone, but on page load, it now reshuffles the options a few times:
2025-04-10.17-13-15.mp4
This might be a feature to signal that the poll options are randomized, but the code isn't written in a way to make this intentional so I consider this bug.
I recommend to just fix it instead of making a feature out of it.
brymut
force-pushed
the
feat/add-random-poll-order
branch
3 times, most recently
from
896b897 to
5352952
Compare
|
Apologies for the back and forth, I've noticed that we don't need anything in the dependency list for the edit: I guess since we're checking for how many times it shuffles we can make it an intentional feature to demonstrate that its shuffling. |
brymut
force-pushed
the
feat/add-random-poll-order
branch
from
5352952 to
1f3f6a0
Compare
|
Changes applied @ekzyis |
|
Any chance you've had a look? @huumn |
|
I'll look at it first thing tomorrow. Thank you for your patience. |
|
Thanks @huumn would be nice to get that first PR merge in. |
| }, | ||
| select: { randPollOptions: true } | ||
| }) | ||
| poll.randPollOptions = pollSettings?.randPollOptions || false |
There was a problem hiding this comment.
The argument item had all the item members including randPollOptions.
I also decided it would be better to randomize on the server so that we don't have to rely on the client doing it. It's not necessarily better, but in general I prefer the server do work so clients are "thinner."
| name='randPollOptions' | ||
| checked={formik?.values.randPollOptions} | ||
| onChange={formik.handleChange} | ||
| />} |
There was a problem hiding this comment.
AdvPostForm is used by every Item form. We have an "escape hatch" for members to be added to the AdvPostForm accordian via the children prop.
There was a problem hiding this comment.
Also the formik stuff was unecessary afaict
| /> | ||
| <Checkbox | ||
| label={<div className='d-flex align-items-center'>randomize order of poll choices</div>} | ||
| name='randPollOptions' |
There was a problem hiding this comment.
checkbox as children like poll expiration.
|
overall, nice work stitching this all together! welcome to the team |
|
Thanks @huumn, enjoyed contributing too. May I ask how the payment of sats is settled? I'm on stacker as [email protected] |
|
Payment will be sent to your lightning address. |
Ok thanks, just seen your test transaction |
|
@huumn any chance the payment will be sorted out today? |
|
I plan on it. |
Description
Added a checkbox to the poll creation form to allow poll creators to choose whether or not the poll choices will be rendered in a random order or not. As discussed here: #2051 (comment)
fixes #2051
Screenshots
Screen recording where I create poll with randomisation activated and I refresh a number of times and then I deactivate and try the same to demonstrate functionality.
rand.mp4
Additional Context
Was anything unclear during your work on this PR? Anything we should definitely take a closer look at?
Checklist
Are your changes backwards compatible? Please answer below:
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
10
For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Checked mobile & light & dark mode.
Did you introduce any new environment variables? If so, call them out explicitly here:
No