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.
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.
For the HashAlignment rule, we're using the default `key` style (keys
are aligned and values aren't) instead of the `table` style (both keys
and values are aligned) because, even if we used both in the
application, we used the `key` style a lot more. Furthermore, the
`table` style looks strange in places where there are both very long and
very short keys and sometimes we weren't even consistent with the
`table` style, aligning some keys without aligning other keys.
Ideally we could align hashes to "either key or table", so developers
can decide whether keeping the symmetry of the code is worth it in a
case-per-case basis, but Rubocop doesn't allow this option.
In the past, whenever we hid users, we also hid their comments.
However, we've now implemented an action to hide users without hiding
their comments. In this case, we still want to show the comment, but we
weren't doing so.
This is useful for people using screen readers, since the character used
as a separator won't be read aloud.
Since many screen readers also read content generated via CSS
pseudoelements, we aren't using `content: "|";` or similar but using
elements with a very small width instead.
Note that in proposal notifications we're writing the call to
render the component in the same line as the <div class="reply">
definition in order to be able to use the `:empty` selector when the
component renders nothing. No browser matches whitespace with the
`:empty` selector, so we can't add newline characters inside the tag. A
more elegant solution would be extracting the proposal notification
actions to a component and only rendering it if the moderation actions
component is rendered.
It was a bit frustrating to click on one of the order elements or the
link to the next page and having to scroll down again until reaching the
comments.
We use order links in many places in the web. However, in the comments
section and the list of community topics, we were displaying a
`<select>` element, and changing the location when users select an
option.
This has several disadvantages.
First, and most important, it's terrible for keyboard users. `<select>`
fields allow using the arrow keys to navigate through their options, and
typing a letter will select the first option starting with that letter.
This will trigger the "change" event and so users will navigate through
a new page while they were probably just checking the available options
[1]. For these reasons, WCAG Success Criterion 3.2.2 [2] states:
> Changing the setting of any user interface component does not
> automatically cause a change of context unless the user has been
> advised of the behavior before using the component.
Second, the form didn't have a submit button. This might confuse screen
reader users, who might not know how that form is supposed to be
submitted.
Finally, dropdowns have usability issues of their own [3], particularly
on mobile phones [4]
The easiest solution is to use the same links we generally use to allow
users select an order, so using them here we make the user experience
more consistent. They offer one disadvantage, though; on small screens
and certain languages, these links might take too much space and not
look that great. This issue affects pretty much every place where we use
order or filter links, so we might revisit it in the future.
Note we're moving the links to order comments after the form to add a
new comment. In my opinion, having an element such as a form to add a
new comment between the element to select the desired order of the
comments and the comments themselves is a bit confusing.
[1] https://webaim.org/techniques/forms/controls#javascript
[2] https://www.w3.org/WAI/WCAG21/Understanding/on-input.html
[3] https://www.youtube.com/watch?v=CUkMCQR4TpY
[4] https://www.lukew.com/ff/entry.asp?1950
This way we can simplify the HTML and easily apply font awesome icons to
any element.
Note the mixin uses `extend`, which we generally try to avoid. It's OK
in this case, since `fa-` classes only have one rule, affecting the
content of its `::before` pseudo-element. Unfortunately we can't use
`include fa-content($fa-var-#{$icon})` because it's not valid SCSS. We
could make the mixin accept an icon instead of an icon name, and call it
using `has-fa-icon(r, $fa-var-plus-square)`. However, IMHO that would
make the code a bit more complex with no real benefit.
The main obstacle to extract this partial was probably the paths for the
flag and unflag actions.
Now that we use Rails 5.1 `resolve` method to handle nested resources,
we can use `polymorphic_path`.
Also note the code is a bit ugly because comments render a divider. We
should probably use a CSS border instead.
Co-Authored-By: taitus <sebastia.roig@gmail.com>
We weren't using `foundation()` in these cases, so after flagging a
debate or a comment, we had to reload the page before we could unflag
it.
We're also adding a test for the fix in commit ea85059d. This test shows
it's necessary to filter the elements with JavaSctipt using `first()` if
we want the same code to work with comments.
Co-Authored-By: taitus <sebastia.roig@gmail.com>
Extract the needed portion of code to a new partial to be able to update
only the elements needed when a new comment is added keeping UI properly
updated.
We were using inline styles and passing local variables around, while
the rule we were following is very simple: it's only hidden if it's a
form to reply to a comment.
We were using a <ul> tag for a single comment, where the first element
of the list was the comment itself and the second element was the list
of replies.
IMHO it makes more sense to have a list of all comments, where every
element is a comment and inside it there's a list of replies.
We're also rendering the list even if it has no children so it's easier
to add comments through JavaScript. Then we use the :empty CSS selector
to hide the list if it's empty. However, since ERB adds whitespace if we
structure our code the usual way and current browsers don't recognize
elements with whitespace as empty, we have to use the `tag` helper so no
whitespace is added.
Tests are also a bit easier to read, even though we need to use the
`text:` option to find links because otherwise the text in the hidden
`<span>` tags will cause `click_link` to miss the link we want to click.
Here's an explanation by one of Capybara's authors:
https://github.com/teamcapybara/capybara/issues/2347#issuecomment-626373440
We were passing around many variables to condition the way we display
the comment. However, in the end we only had one place where these
variables were used: valuation. So we can make everything depend on the
valuation variable.
It was created in commit 83d254ad, but it was never used, since the
commit creating it removed the code rendering the
`budgets/investments/comments` partial, which this partial was supposed
to replace.
They do the exact same thing; however `html_safe` might confuse
developers into thinking it will make the HTML safe. Using `raw` makes
it clear that we're inserting the text without escaping it.
Sometimes we're interpolating a link inside a translation, and marking
the whole translations as HTML safe.
However, some translations added by admins to the database or through
crowdin are not entirely under our control.
Although AFAIK crowdin checks for potential cross-site scripting
attacks, it's a good practice to sanitize parts of a string potentially
out of our control before marking the string as HTML safe.
The name `safe_html_with_links` was confusing and could make you think
it takes care of making the HTML safe. So I've renamed it in a way that
makes it a bit more intuitive that it expects its input to be already
sanitized.
I've changed `text_with_links` as well so now the two method names
complement each other.
I'm not sure why it isn't already done by foundation's form builder. It
doesn't make any sense to change an ID of a form field without changing
the `for` attribute of its label.
Using the block syntax to generate the label with a <span> tag inside
isn't necessary after upgrading foundation_rails_helpers. Before the
upgrade, we couldn't do so because the <span> tag was escaped.
