We were displaying the total number of notifications with a message "You
have N unread notifications", but were using the total number of
notifications instead of the unread ones.
It was hard to know where the numbers were coming from; they depended on
the padding of the link and the size of the notification icon size. So
we're using variables to make it more explicit.
However, the code is now too complex, so we'll probably have to simplify
it in the future.
Other than simplifying the view, we can write tests using `click_link`,
which makes the tests more robust. Clicking the `.icon-notification`
element was causing some tests to fail when defining a window height of
750 pixels in the `admin_budgets` branch.
Hopefully now it's a bit more obvious that SDGs can be selected by
clicking on them and that the field to select goals and the field to
select targets/goals are related (since they're now part of the same
fieldset).
We were using list items with the checkbox role. This is probably
confusing since list items have a listitem role, and so we were
basically using a list with no listitem.
We could add a `<span role="checkbox">` tag inside the list item, but
using real checkboxes is probably easier. We're also adding a test to
verify the checkboxes native behavior is compatible with our code.
Note we're using the "enter" key to toggle the "checked" status of the
SDG. This is probably not intuitive for screen reader users who might
try to submit the form using the "enter" key after selecting a goal.
However, the alternative would be even less intuitive for sighted
keyboard users, since for them these icons look like links or buttons
and they would accidentally submit the form when trying to select a
goal.
Since we haven't come up with a better interface yet, for now we're
following the principle of least damage; we consider submitting the form
against a user's will is less annoying than forcing users to move to a
different field before being able to submit the form.
Also note we can't write `check` in the tests because Capybara will try
to click the checkbox, which is hidden by the image in the label.
These styles were broken in commit 2614de79c, since there we only
considered scenarios where the `tags` element is a list. In these forms, however, the `tags` element is a regular <div> tag.
Since we've changed the facebook and twitter login icons, it makes sense
to use the same icons in social share links.
Besides, using font-awesome simplifies the code and prevents problems
with screen readers announcing the value of the `content:` CSS property.
We were using the icon for google plus, which was confusing since google
plus no longer exists.
Note this change requires changing the font for these icons, since the
google icon is not present in the "icons" font. On the plus side, using
the font awesome icons we can simplify the code a little bit.
We had one exception running test suite [1]:
```
1) Commenting legislation questions Merged comment threads View
comments of annotations in an included range
Failure/Error: count: annotation.comments.roots.count) %>
ActionView::Template::Error:
PG::ProtocolViolation: ERROR: bind message supplies 0
parameters, but prepared statement "" requires 2
: SELECT COUNT(*) FROM "comments" WHERE "comments"."hidden_at" IS
NULL AND "comments"."commentable_id" = $1 AND
"comments"."commentable_type" = $2 AND "comments"."ancestry" IS NULL
```
Debugging shows this test was run right after the flaggable specs.
There's a chance these exceptions take place because the test is
accessing the database after the browser (chromedriver) process has
already accessed the database.
This is just an attempt at fixing the issue; I don't know whether these
changes will fix it since I've only seen this exception on Github
Actions (never on my machine). Worst case scenario, we're encouraging
good practices of system tests: test things from the user's point of
view.
[1] https://github.com/consul/consul/runs/1856245992
Using a button tag, it's possible for every user to "click" the element.
Besides, we don't need to call the `preventDefault` function, because
buttons with type "button" don't do anything by default.
The `proposal-show` HTML class was only used in this context to style
the whatsapp icon, which is now styled the same way as other social
share icons. The `proposal-show` class should be reserved for the actual
proposal show action.
We were using a custom icon because in the past social-share-button
didn't have support for whatsapp. But now that it does, we can remove
our custom icon.
Note we're using the `_app` suffix because that's the name of the icon
meant for mobile devices.
We've had a failure in one of our test suite runs [1]. A possible cause
could be an HTTP request taking place at the same time as an AJAX
request, with both trying to access the database at the same time.
We've had several similar issues in the past which have been fixed by
checking the AJAX request has finished before requesting another page,
so we're applying the same principle here.
[1] https://github.com/consul/consul/runs/1855207922
We were doing it manually, but Capybara offers an option which does the
exact same thing.
This way we also apply the NoJavascriptTagHelper ERB rule, which
reported one error in the `disable_animations_in_tests` partial.
The association `organization.user` returns `nil` when the user is
hidden.
This was discovered thanks to the `Style/AndOr` rule. We were using
`and` and `||` on the same line, which is confusing.
This way we can simplify the code and don't have to rely on `.try`
statements which are confusing and so we don't allow them in the
`Rails/SafeNavigation` Rubocop rule.
Note that in Ruby files this rule allows vertical alignment, but doesn't
seem to do the same in ERB. Since we only used vertical alignment in one
place, and that place also had an unneeded extra space on every aligned
line, I've decided to change the code in that place and follow the rule.
So now we remove duplication between .rubocop.yml and the rubocop rules
defined for erblint. Having the rules in two places led to some
mistakes, like renaming Layout/Tab to Layout/IndentationStyle in
`.rubocop.yml` but forgetting to do so in `.erb-lint.yml`.
This also means we can use the EnforcedStyle options for
Layout/SpaceInsideHashLiteralBraces in views as well.
We couldn't implement this feature earlier because it required Ruby 2.4
and due to incompatibilities between versions of erb_lint and versions
of rubocop.
There are some rules which do not apply to ERB files and so we're
disabling them.
* Layout/LineLength, Layout/TrailingEmptyLines and Lint/UselssAssignment
generate false positives
* Rails/OutputSafety is redundant since its functionality is already
covered by ERB Lint ErbSafety linter
