chore(vote): stubbed out voting #209

nguernse · 2024-04-24T01:29:49Z

Pull Request Template

Fixes:
Closes:

Checklist

This PR conforms to the guidelines in CONTRIBUTING.md
- It has a descriptive title
- My commits conform to the conventional commits specification
This PR is not a duplicate of a similar PR (how to search issues for similar/duplicate PRs, which this is not
This PR targets the dev branch
I ran the pre-commit hooks or npm test && npm lint && npm test:e2e and all tests pass

Description

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Code cleanliness (removal of unnecessary comments or debug logs)

How Has This Been Tested?

Microservices and API Integration

If applicable, my changes are compatible with the project's microservices architecture and API gateway integration requirements.

Checklist:

My code follows the style guidelines of this project
I have made corresponding changes to the documentation
I have removed all unnecessary comments and debug log statements
I have linked this PR to its corresponding issue

Screenshots (if appropriate):

Please review and suggest any further insights, improvements, or modifications needed.

helmturner

PRAISE: You really got a lot done! That's two major parts of this feature you've improved in less than a week. We're so close to launching!
ISSUE(blocking): This needs updating in light of the changes in #236.
SUGGESTION: Might be easiest to split each component into it's own file, and then rebase on top of #236 (or main, if it's merged). That way you have fewer merge conflicts and can compare before/after a little easier.
NOTE: Happy to do this for you, if you prefer!

helmturner · 2024-04-29T06:21:53Z

I took the liberty of handling the merge conflicts and bringing this up to date with main, but I didn't want to force push over your pr branch without giving you a chance to review it first.

There's a copy of the results at https://github.com/tulsawebdevs/website/tree/feature/proposal-voting in case you'd like to compare them. If that branch looks good to you, let me know and I'll push it to this PR.

nguernse · 2024-04-29T18:54:09Z

@helmturner Awesome, thanks for doing that! I had a look and everything looks good to me.

nguernse · 2024-04-30T17:13:15Z

@helmturner I had some time today, so hope you don't mind I went ahead and incorporated your updates into this PR. I then also added

debouncing voting
preventing votes without being authenticated
a few UI updates to proposal card

Regarding the voting data: Will the user's voting data (upvotes and totals depending on proposal status) be included with the proposal data or will we be needing to fetch that on a per-proposal basis?

helmturner · 2024-04-30T17:19:21Z

Oh nice, thank you! I'm sorry I didn't get around to it sooner, some family stuff came up that I had to focus on.

Re: votes, I think the idea will be to have an endpoint specifically for user's votes. That would be 1 query for user's voting record (maybe we pull the whole thing and put it in client state), and then when a proposal/proposal list is queried we can just populate the vote totals from state.

I'll get this reviewd right now 🖖

helmturner

PRAISE: excellent work again! Always a pleasure reviewing your code.
POLISH: I think this is g2g after just some minor touch-ups.
QUESTION: If you've got any opinions / curiosity about the form/fetching infra, I've got an experimental branch where I'm exploring some options.

helmturner · 2024-04-30T18:09:18Z

src/features/vote/proposals/list/ProposalListItem.tsx

+  const handleVote = useProtectedFunction(
+    useDebounce((value: Vote) => {
+      const votePayload: VotePayload = {
+        initiativeId: proposal.id.toString(10),
+        vote: value,
+        comment: '',
+        authorId: proposal.authorId,
+        authorName: proposal.authorName,
+        authorEmail: proposal.authorEmail,
+      };
+
+      setVoteValue(value);
+      voteRequest(votePayload).catch(() => {
+        toast.error('Could not cast vote. Please try again.');
+      });
+    }),
+    {
+      unauthorizedMessage: 'You do not have permission to vote.',
+    },
+  );


ISSUE: We probably want to take the setVoteValue(value) call out of the debounce to improve perceived performance. As it is, some clicks are 'ignored' from the user's perspective.

FIX: If I'm not mistaken, we should just be able to lift the function call out of the debounce hook without causing the value to fall out of sync with what's sent in the request.

NOTE(non-blocking): Normally we'd want to store the user's initial vote in memory and only update it when we get a success confirmation from the server. That way we can roll back the UI if the vote request fails. No need to worry about it here, though; i'm researching some libraries to use for data fetching that should abstract most of the query management cycle away.

That makes sense. I thought it was awkward to see a slight delay. I've lifted up setting the vote value.

helmturner · 2024-04-30T18:19:34Z

src/features/vote/proposals/list/ProposalInterestVote.tsx

+const voteOptions = [
+  {
+    label: 'Strongly Disinterested',
+    value: '-2',
+    id: 'vote-strongly-disinterested',
+  },
+  {
+    label: 'Slightly Disinterested',
+    value: '-1',
+    id: 'vote-slightly-disinterested',
+  },
+  { label: 'Neutral', value: '0', id: 'vote-neutral' },
+  { label: 'Slightly Interested', value: '1', id: 'vote-slightly-interested' },
+  { label: 'Strongly Interested', value: '2', id: 'vote-strongly-interested' },
+];


POLISH: I think we can shorten some of the labels on these
SUGGESTION:

Suggested change

const voteOptions = [

{

label: 'Strongly Disinterested',

value: '-2',

id: 'vote-strongly-disinterested',

},

{

label: 'Slightly Disinterested',

value: '-1',

id: 'vote-slightly-disinterested',

},

{ label: 'Neutral', value: '0', id: 'vote-neutral' },

{ label: 'Slightly Interested', value: '1', id: 'vote-slightly-interested' },

{ label: 'Strongly Interested', value: '2', id: 'vote-strongly-interested' },

];

const voteOptions = [

{

label: 'Strong Disinterest',

value: '-2',

id: 'vote-strong-disinterest',

},

{

label: 'Slight Disinterest',

value: '-1',

id: 'vote-slight-disinterest',

},

{ label: 'Neutral', value: '0', id: 'vote-neutral' },

{ label: 'Slight Interest', value: '1', id: 'vote-slight-interest' },

{ label: 'Strong Interest', value: '2', id: 'vote-strong-interest' },

];

I like this suggestion! I've updated based on input.

helmturner · 2024-04-30T18:23:08Z

src/features/vote/proposals/list/ProposalInterestVote.tsx

+        return (
+          <Label
+            key={`${proposal.id}-${option.id}`}
+            className={cn('flex items-center gap-2', {


POLISH: The radio buttons look pretty wonky to me. That's mostly because we have an odd number of off-balance items, but I wonder moving the labels below the radio buttons might help it look more even?
SUGGESTION:

Suggested change

className={cn('flex items-center gap-2', {

className={cn('flex w-[20%] flex-col text-center items-center gap-2', {

You're right, I agree with that. I've made an update to reflect this.

helmturner

PRAISE: I have nothing to add... this is just great. Beautiful work! Love the code-quality improvements.

helmturner linked an issue Apr 26, 2024 that may be closed by this pull request

feat: Proposal Listing Page/Index #85

Closed

zenlex deleted the branch tulsawebdevs:main April 27, 2024 08:08

zenlex closed this Apr 27, 2024

zenlex reopened this Apr 27, 2024

zenlex changed the base branch from dev to main April 27, 2024 16:51

zenlex force-pushed the feature/proposal-voting branch 2 times, most recently from ef0bc68 to 45e25a8 Compare April 27, 2024 17:53

helmturner removed a link to an issue Apr 28, 2024

feat: Proposal Listing Page/Index #85

Closed

helmturner linked an issue Apr 28, 2024 that may be closed by this pull request

feat: Voting Component #96

Closed

helmturner requested changes Apr 28, 2024

View reviewed changes

chore: refactor proposal voting ui

a210e66

nguernse added 5 commits April 29, 2024 10:07

chore: remove by line

b1af749

chore: display proposal status

47d5377

chore: sync voting between buttons and radio group

aa94d1f

chore: remove deprecated types

03fb86f

chore(vote): debounce voting function

be10855

nguernse added 5 commits April 29, 2024 14:55

chore: move vote elements to separate components

aeaeb5d

chore: no cursor on disabled

433d8c0

chore: added authenticated function hook

f99b73e

chore: remove deprecated function

a0ce04b

fix: update api url

1cbb0cd

nguernse force-pushed the feature/proposal-voting branch from 45e25a8 to 1cbb0cd Compare April 30, 2024 16:45

nguernse added 3 commits April 30, 2024 11:47

Merge branch 'main' into feature/proposal-voting

44dd7fb

chore: update to use new toast component

5088ac9

fix: linting errors

740a5ea

helmturner requested changes Apr 30, 2024

View reviewed changes

nguernse added 3 commits April 30, 2024 14:07

chore: update radio styling

9f4cae3

chore: lift setVote outside debounce

6e58c44

chore: update typings

9ed3452

nguernse requested a review from helmturner April 30, 2024 20:22

helmturner approved these changes May 3, 2024

View reviewed changes

helmturner merged commit b634181 into tulsawebdevs:main May 3, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(vote): stubbed out voting #209

chore(vote): stubbed out voting #209

nguernse commented Apr 24, 2024

helmturner left a comment •

edited

Loading

helmturner commented Apr 29, 2024

nguernse commented Apr 29, 2024

nguernse commented Apr 30, 2024

helmturner commented Apr 30, 2024

helmturner left a comment

helmturner Apr 30, 2024 •

edited

Loading

nguernse Apr 30, 2024

helmturner Apr 30, 2024

nguernse Apr 30, 2024

helmturner Apr 30, 2024

nguernse Apr 30, 2024

helmturner left a comment

	className={cn('flex items-center gap-2', {
	className={cn('flex w-[20%] flex-col text-center items-center gap-2', {

chore(vote): stubbed out voting #209

chore(vote): stubbed out voting #209

Conversation

nguernse commented Apr 24, 2024

Pull Request Template

Checklist

Description

Type of Change

How Has This Been Tested?

Microservices and API Integration

Checklist:

Screenshots (if appropriate):

helmturner left a comment • edited Loading

Choose a reason for hiding this comment

helmturner commented Apr 29, 2024

nguernse commented Apr 29, 2024

nguernse commented Apr 30, 2024

helmturner commented Apr 30, 2024

helmturner left a comment

Choose a reason for hiding this comment

helmturner Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

nguernse Apr 30, 2024

Choose a reason for hiding this comment

helmturner Apr 30, 2024

Choose a reason for hiding this comment

nguernse Apr 30, 2024

Choose a reason for hiding this comment

helmturner Apr 30, 2024

Choose a reason for hiding this comment

nguernse Apr 30, 2024

Choose a reason for hiding this comment

helmturner left a comment

Choose a reason for hiding this comment

helmturner left a comment •

edited

Loading

helmturner Apr 30, 2024 •

edited

Loading