Just like we do everywhere else. We're also removing the wrong ARIA
attributes that we added in commit c18479e3a, which caused an
accessibility issue reported by Axe:
```
aria-required-children: Certain ARIA roles must contain particular
children (critical)
https://dequeuniversity.com/rules/axe/4.10/aria-required-children?application=axeAPI
The following 1 node violate this rule:
Selector: .tabs-content
HTML: <div class="tabs-content"
data-tabs-content="information-texts-tabs" role="tablist">
Fix any of the following:
- Element has children which are not allowed: ul[tabindex]
```
Although, in this case, it would probably be better to have different
pages instead of tabs, so loading the page doesn't take too long.
Using a `data-toggle` attribute, which we do since commit 07fd5084f,
made Foundation generate an `aria-expanded` attribute to a radio button,
but this attribute can't be present in radio buttons. This makes sense,
since the main purpose of a radio button in a form is to choose an
option, not to show/hide content.
This resulted in the following error when checking the page with Axe:
```
Found 1 accessibility violation:
aria-allowed-attr: Elements must only use supported ARIA attributes
(critical)
https://dequeuniversity.com/rules/axe/4.10/aria-allowed-attr?application=axeAPI
The following 2 nodes violate this rule:
Selector: #dashboard_action_action_type_proposed_action
HTML: <input data-toggle="request_to_administrators short_description"
type="radio" value="proposed_action" checked="checked"
name="dashboard_action[action_type]"
id="dashboard_action_action_type_proposed_action"
aria-expanded="true"
aria-controls="request_to_administrators">
Fix all of the following:
- ARIA attribute is not allowed: aria-expanded="true"
Selector: #dashboard_action_action_type_resource
HTML: <input data-toggle="request_to_administrators short_description"
type="radio" value="resource"
name="dashboard_action[action_type]"
id="dashboard_action_action_type_resource"
aria-expanded="true"
aria-controls="request_to_administrators">
Fix all of the following:
- ARIA attribute is not allowed: aria-expanded="true"
```
So we're using custom JavaScript instead. We're also making the
`short_description` field act as intended; since the changes in commit
07fd5084f it was never shown because it had the `hide` HTML class and it
didn't have a `data-toggler` attribute.
We can extract a method to reduce the amount of ERB code and remove the
duplication in the link texts. We also make the list consistent; now we
always use a <strong> tag in the group name, no matter whether there are
many groups or only one.
The original implementation (which was never merged) had a `<select>`
field for the switch, which offered accessibility issues. So I came up
with a very bad idea, which was emulating the look and feel of a select
field while making it more accessible for keyboard users.
This approach is inconvenient because we were using a bunch of ARIA
roles to do the same thing that can be done with a list of links, going
against the first rule of ARIA, which is:
> "Don’t use ARIA if you can achieve the same semantics with a native
> HTML element or attribute
Not only that, but the control was confusing for people using mobile
phones (select fields don't behave the same way), and we were using
*invalid* ARIA roles in this situation, leading Axe to report a critical
accessibility error:
```
aria-required-children: Certain ARIA roles must contain particular
children (critical)
https://dequeuniversity.com/rules/axe/4.10/aria-required-children?application=axeAPI
The following 1 node violate this rule:
Selector: ul[data-dropdown-menu="edw1i2-dropdown-menu"]
HTML: <ul class="dropdown menu" wnenu="edw1i2-dropdown-menu"
data-disable-hover="true" op="true" role="menubar">
Fix any of the following:
- Element has children which are not allowed: button[tabindex]
```
So, at least for now, we're using a simple list of links. We might style
it in the future if we find ways to make usability improvements, but,
for now, it does the job, and it does it better than the custom control
we were using.
We were getting an exception in this case, which was OK I guess since
this shouldn't happen if the application is used in a normal way, but we
can simplify the code a little bit if we make the `recipients` code
return an empty list of users.
Note that the behavior of the `AdminNotification#list_of_recipients` and
`Newsletter#list_of_recipient_emails` methods is now slightly different;
previously they returned `nil` when given an invalid segment recipient,
while now they return an empty array. I haven't found a place where this
change is relevant. For example, in both of these models, the `deliver`
method used to raise an exception when given an invalid segment while
now it doesn't, but we always check the user segment is valid before
calling the `deliver` method anyway, so it doesn't really affect the
application.
We were getting a warning by CodeQL regarding a possible code injection
in the `send(segment)` code.
In practice, this wasn't a big deal because the `self.recipients` method
is only called in the admin section, meaning only admin users could try
to take advantage of the code injection, and because this code is rarely
called with an invalid segment due to several conditions in the code
checking that the user segment is valid, with the only exception being
the `generate_csv` action in the `Admin::EmailsDownloadController`.
In any case, now we're checking that the segment is valid before calling
the `send` method. Since now we're making sure that the segment is valid
in the `recipients` method itself, we can remove this check from methods
calling it.
This way we can move some system tests to component tests and stop
creating records after starting the browser with a `visit`.
We could also split the system test in two, but since these tests
aren't checking any user interactions, moving the to component tests we
check the same things while making the tests faster.
Since the partial was using an instance variable, we're passing it to
the component. We're naming it `voter_user` instead of `user` because
passing something named `user` could make us think that we're passing
the `current_user`. I wasn't sure about naming it `voter` because it's a
`User` record and not a `Poll::Voter` record, but naming it `voter`
would definitely be an option.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
In this case, we were hiding a proposal after starting the process
running the browser to check what happens when accessing a notification
for a hidden proposal. We can avoid database access in the middle of the
test by hidding a proposal before starting the browser. The process to
create a notification using the browser is already tested in other
specs, so we don't need to do it here as well.
Note that, to simplify the test, we're extracting the `notify_users`
method. I wonder whether this method should be called in an
`after_create` callback instead... That's a topic for another time,
though.
I don't think this feature it was ever used. It was introduced in commit
49dec6061 as part of a feature that was removed in commits 1cd47da9d and
c45a0bd8ac.
We removed the link to this page in commit 83e8d6035 because poll
questions don't really make sense without a poll.
However, this page also contained information about successful
proposals, which might be interesting so administrators don't have to
navigate to the public area in order to find and create questions based
on successful proposals.
So we're keeping the part about successful proposals and linking it from
the proposals part of the admin area.
Note we're using translation keys like `successful_proposals_tab`, which
don't make sense anymore, for the successful proposals. We're doing so
because we've already got translations for these keys and, if we renamed
them, we'd lose the existing translations and our translators would have
to add them again.
Also note we're changing one poll question test a little bit so we
create the question from a successful proposal using the new page. There
are other tests checking how to create a question from the
admin/proposals#show action and other tests checking what happens when
accessing a successful proposal in the admin section, so we don't lose
any test coverage by changing an existing test instead of adding a new
one.
Finally, note that we've removing the `search` method in poll question
because we no longer use it. This currently makes the
`author_visible_name` database column useless; we aren't removing it
right now because we don't want to risk a possible data loss in a patch
release (we're about to release version 2.3.1), but we might remove it
in the future.
These rules aren't used since commit c5c9efee1.
Note that there's still an `icon-budget` element in the
`_investment_show` partial. The rules for this icon are already defined
inside the rules for the `.budget-investment-show` selector.
The `icon-budget` and `icon-proposals` HTML classes are still used in
the "Following" tab (the code uses the `followable_icon` method), but in
this case the `.budget-investment` or `.proposal` selectors aren't
present, so the properties weren't applied here either.
Similarly, there are elements with the `icon-debates` and
`icon-proposals` HTML classes in the dashboard area, but they aren't
inside elements matching the `.debate` or `.proposal` selectors either.
And there's an element with the `icon-debate` HTML class in the
legislation area. Once again, it isn't inside an element matching the
`.debate` selector.
So can safely remove this code.
This column isn't used since commit 4c0deb0ec because administrators can
associate videos to the answers since commit 5862eea51. The value of
this attribute isn't used in the public area since commit 8277e3cc2.
This parameter isn't used since commit b4a6f664b.
Note we're changing the tests to use proposals instead of debates
because proposals may have images attached, while debates may not.
This way we're also checking mistakes like closing tags that don't match
their opening element, which we detected by manually running HTML
Beautifier with the `-e` option, and fixed two commits ago.
Note there was a false positive in the mailer layout. We don't know the
cause. Maybe closing the ERB tag right before the HTML opening tag and
the lack of other attributes on the tag made HTML Beautifier think the
tag wasn't correctly open, but on the other hand, we have the exact same
line in other layouts where HTML Beautifier works fine. We're fixing it
by adding an HTML id attribute to the element.
We had inconsistent indentation in many places. Now we're fixing them
and adding a linter to our CI so we don't accidentally introduce
inconsistent indentations again.
In some places, we accidentally opened the same tag twice instead of
closing it, while in some other places we closed a tag without opening
it in the first place.
We've detected these issues thanks to the HTML Beautifier gem, which
we're about to start using for indentation purposes.
Using an `if..else` block made the code harder to follow since the
opening tag was inside the block but the closing tag was outside it.
Moreover, it didn't work well with HTML Beautifier (a gem we're going to
introduce to manage ERB indentations).
This way we simplify the HTML, which had some `if...else` blocks that
were hard to follow because there were opening tags inside these blocks
while the closing tags were outside these blocks.
We're also making the CSS container-dependent instead of
window-dependent. Since there are between one and three elements inside
the panel, we accomplish this by making the element with the content
take its own line if the width of the panel is smaller than 35rem.
Note we're trying to keep the layout similar to what it was; since we're
no longer using negative margins (like the ones in the `.row` selector),
the votes element now gets a width of 22.5% instead of 25%.
Also note we're using the column-gap property for flexbox because the
`flex-with-gap` mixin doesn't work so well with elements that have
borders. Since the column-gap property for flexbox is now supperted by
more than 98% of the browsers (which wasn't the case when we started
using the `flex-with-gap` mixin), the `flex-with-gap` mixin has become
obsolete.
Finally, note we're removing the `max-width: 12rem` rule in the images.
I'm not sure why we introduced this rule in the first place, and it
didn't play so well to the new layout. I considered using code like
`max-width: min(100%, 12rem)`, but, since I'm not sure why `12rem` was
there in the first place, I'm not sure whether this approach was better,
and it sure made things more complex.
Not doing so made it trickier to define a flex layout, since the
icon-successful element is given a `position: absolute`, but only for
successful proposals, while for unsuccessful proposals it was taking
the standard `position: static` value.
We're also reusing the `successful?` method instead of rewriting it in
the view, and fixing a small issue where the icon wasn't displayed for
proposals having the exact needed votes (we were using `>` instead of
`>=` in the condition).
Note that legislation proposals use the method
`Proposal.votes_needed_for_success`, which is probaby a mistake caused
by copying the code from the proposal view. Fixing this issue is out of
the scope of this commit (and pull request), though.
With the multiline condition, HTML Beautifier (which we're about to
introduce in order to manage ERB indentation) gets confused. In this
context, a one-line condition is also more readable.
We had some duplication because the `css_for_process_header` was using
an instance variable, and so it couldn't be called from a partial where
this instance variable wasn't available.
Using a local variable and passing it as a parameter (as we should
always do) solves the issue and lets us simplify the code.
* The `gender-and-channel` class isn't use since commit e3063cd24f
* The `first-comment` class added in commit e5fb90f954 was never used
* The `additional-info` class added in commit d47ce39b37 was never used
* The `title-quote` class isn't use since commit f317514258
* The `big-number` class added in commit aa1ed87725 was never used
* The `big-number-stat` class isn't used since commit cf32dcd6d5
* The `budget-investment-amount` and `budget-investment-description`
classes added in commit 4ade8325d7 were never used
* The `debate-quiz` class isn't use since commit d679c1eb79
* The `enquiries-list` class isn't use since commit 6aa96c4471
* The `investment-project-content` class isn't use since commit d0b8fef6b3
* The `leave-comment` class isn't use since commit ff6285d114
* The `poll-question-show` class isn't use since commit 8277e3cc2b
* The `questions-callout` class isn't use since commit ddf8d5f811
* The `budget-investments-inner` class isn't used since commit c6a9ffc19
* The `map-attributtion` class isn't use since commit 5a09b79da4
* The `postfix` class isn't use since commit 3752f3a53b
* The `translation-locale` added in commit a6e8fecc16 was never used
* The `changes-link`, `comment-advice`, `show-comments` and `comment-votes-number`
classes aren't use since commit d679c1eb79
This stylelint-scss rule is useful because we were inconsistent when
using calc(); sometimes we added interpolation to Sass variables, and
sometimes we didn't. The reason why we originally added interpolation
was that it was necessary until we migrated to Dart Sass in commit
d54971e53. Since then, we can omit the interpolation, which is also what
the Sass documentation recommends [1].
[1] https://sass-lang.com/documentation/values/calculations/
We made a mistake when adding the `calc()` function in commit 6df813fdb,
since the `/` operator originally only affected the `$heading-icon-size`
part of the operation, but affected the whole operation after that
commit. This caused the icon to be positioned on top of another icon.
The interpolation of the rem-calc function made the code harder to read.
Besides, for new code we've already agreed upon using rem units instead
of `rem-calc`.
For instance, we had the following code:
```
width: calc(100% - #{2 * rem-calc(10)});
```
Now, `2 * rem-calc(10)` is the same as `rem-calc(20)`, but we were using
`2 *` to make it clear that this value was related to the value of the
`margin-left` property, which was `rem-calc(10)`.
IMHO using `0.625rem` for the margin and `2 * 0.625rem` for the width
the code is easier to read because there are no interpolation and no
complex operations involved.
We forgot to add the `calc()` function in commits 4c0b6455f, 390c749d2
and dc54fda71.
We were getting a warning when compiling the assets without the
`quiet_deps` option:
```
DEPRECATION WARNING: Using / for division outside of calc() is
deprecated and will be removed in Dart Sass 2.0.0.
Recommendation: math.div($line-height, 3) or calc($line-height / 3)
More info and automated migrator: https://sass-lang.com/d/slash-div
```
Set cookie duration to 365 days based on the AEPD's cookie usage guidelines.
Note from the document: "Cookies with a duration of up to 24 months are
considered acceptable as long as they are periodically updated."
Reference: https://www.aepd.es/guias/guia-cookies.pdf
Create cookie consent "all" when accept all cookies
Set cookie duration to 365 days based on the AEPD's cookie usage guidelines.
Note from the document: "Cookies with a duration of up to 24 months are
considered acceptable as long as they are periodically updated."
Reference: https://www.aepd.es/guias/guia-cookies.pdf
