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

Voting response improvements #297

Closed
umputun opened this issue Mar 25, 2019 · 3 comments · Fixed by #303
Closed

Voting response improvements #297

umputun opened this issue Mar 25, 2019 · 3 comments · Fixed by #303

Comments

@umputun
Copy link
Owner

umputun commented Mar 25, 2019

The current implementation of the voting delegates too much work to the client, sends a lot of useless data and exposes too many details. In fact, it sends back to the client the full list of all votes with user ids and +/- status to allow UI handle voting status, colors and so on. Such a list can be huge and we had to restrict maximum size (see #207).

The response (i.e. Comment) can be simplified by stripping votes map completely and replacing it by singular vote field indication +/- for the current user.

@igoradamenko & @Reeywhaar - do you see any potential problem I can cause to UI by making the change? Sure, it will need adjustment to read vote status, but to me, it sounds like a trivial change and even simplification of UI logic.

@Reeywhaar
Copy link
Collaborator

I've thought recently that it possible for a user to find who up/downvoted them by looking at response and matching user id's. I think, yes, {vote: {score: number, delta: -1/0/+1}} value is enough to handle voting.

@umputun
Copy link
Owner Author

umputun commented Mar 25, 2019

I didn't plan to touch the existing score field, but just adding vote : [0, -1, 1]

@umputun
Copy link
Owner Author

umputun commented Mar 25, 2019

@Reeywhaar - I have a functional version in vote branch with votes field stripped and new vote =1,-1,0 added

I can't merge it to master because it will break voting on UI side, but you may add your PR and test with vote branch. We have to release both at the same time.

@umputun umputun mentioned this issue Apr 10, 2019
@umputun umputun added this to the v1.3 milestone Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants