-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow undoing "like/unlike" votes #5118
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spec/controllers/legislation/proposals/votes_controller_spec.rb
Outdated
Show resolved
Hide resolved
spec/controllers/legislation/proposals/votes_controller_spec.rb
Outdated
Show resolved
Hide resolved
spec/controllers/legislation/proposals/votes_controller_spec.rb
Outdated
Show resolved
Hide resolved
spec/controllers/legislation/proposals/votes_controller_spec.rb
Outdated
Show resolved
Hide resolved
In order to the users using screen readers know whether the button is pressed or not.
ae546ed
to
9da43c0
Compare
javierm
reviewed
Oct 6, 2023
In order to reduce the code used to add styles to the buttons, we removed the classes that had been added and handled it with the new aria-pressed attribute
In order to the users using screen readers know whether the button is pressed or not.
In this commit, we have performed a refactoring to enhance code organization. Several partials that were solely responsible for rendering components have been removed. Instead, we are now directly rendering the components within the views where these partials were previously used.
javierm
reviewed
Oct 6, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing I forgot about 😉.
As far as possible I think the code is clearer if we use CRUD actions rather than custom actions. This will make it easier to add the action to remove votes in the next commit. Note that we are adding this line as we need to validate it that a vote can be created on a debate by the current user: ```authorize! :create, Vote.new(voter: current_user, votable: @debate)``` We have done it this way and not with the following code as you might expect, as this way two votes are created instead of one. ```load_and_authorize_resource through: :debate, through_association: :votes_for``` This line tries to load the resource @debate and through the association "votes_for" it tries to create a new vote associated to that debate. Therefore a vote is created when trying to authorise the resource and then another one in the create action, when calling @debate.vote_by (which is called by @debate.register_vote).
Since this commit 6e27917 it seems that it is no longer necessary to continue using shallow.
As far as possible I think the code is clearer if we use CRUD actions rather than custom actions. This will make it easier to add the action to remove votes in the next commit. Note that we are adding this line as we need to validate it that a vote can be created on a comment by the current user: ```authorize! :create, Vote.new(voter: current_user, votable: @comment)``` We have done it this way and not with the following code as you might expect, as this way two votes are created instead of one. ```load_and_authorize_resource through: :comment, through_association: :votes_for``` This line tries to load the resource @comment and through the association "votes_for" it tries to create a new vote associated to that debate. Therefore a vote is created when trying to authorise the resource and then another one in the create action, when calling @comment.vote.
Co-authored-by: Javi Martín <[email protected]>
javierm
approved these changes
Oct 9, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 👏
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives
Visual Changes
Undo vote:
undo_vote.mov
Undo vote on comments:
undo_vote_comments.mov