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.
We were rendering one label and many textarea fields for that label.
This meant that, when switching to a different language, the label
wasn't correctly associated with the textarea.
So we're now rendering one label for each textarea. We could use
`aria-label` or `aria-labelledby` instead, but using a label offers some
advantages like the fact that clicking on the label makes the textarea
take the focus.
Just like we do every else (sometimes even on that very same file), we
use the method instead of the instance variable.
We're doing this change now because we're about to modify one of these
files (the poll question answers documents index component).
Note that, currently, we take these settings from the database but we
don't provide a way to edit them through the admin interface, so the
locales must be manually introduced through a Rails console.
While we did consider using a comma-separated list, we're using spaces
in order to be consistent with the way we store the allowed content
types settings.
The `enabled_locales` nomenclature, which contrasts with
`available_locales`, is probably subconsciously based on similar
patterns like the one Nginx uses to enable sites.
Note that we aren't using `Setting.enabled_locales` in the globalize
initializer when setting the fallbacks. This means the following test
(which we could add to the shared globalizable examples) would fail:
```
it "Falls back to an enabled locale if the fallback is not enabled" do
Setting["locales.default"] = "en"
Setting["locales.enabled"] = "fr en"
allow(I18n.fallbacks).to receive(:[]).and_return([:fr, :es])
Globalize.set_fallbacks_to_all_available_locales
I18n.with_locale(:fr) do
expect(record.send(attribute)).to eq "In English"
end
end
```
The reason is that the code making this test pass could be:
```
def Globalize.set_fallbacks_to_all_available_locales
Globalize.fallbacks = I18n.available_locales.index_with do |locale|
((I18n.fallbacks[locale] & Setting.enabled_locales) + Setting.enabled_locales).uniq
end
end
```
However, this would make it impossible to run `rake db:migrate` on new
applications because the initializer would try to load the `Setting`
model but the `settings` table wouldn't exist at that point.
Besides, this is a really rare case that IMHO we don't need to support.
For this scenario, an installation would have to enable a locale, create
records with contents in that locale, then disable that locale and have
that locale as a fallback for a language where content for that record
wasn't created. If that happened, it would be solved by creating content
for that record in every enabled language.
This parameter isn't used since commit b86c0d3c3, which deleted a file
that wasn't used since commit 146c09adb. Further proof that this code
wasn't used is the fact that the `enable_translation_style` method,
which this code called, was removed in commit 5ada97544.
Note that we used to have the link to delete images inside the same
<form> tag as the button to update the image. However, using a button
means we're adding a new <form> tag for the action to delete the image.
This isn't valid HTML and, in some browsers, might result in the button
sending the request to the wrong URL.
As explained in commit 5311daadf, to avoid this, we'd need to replace
`button_to` with `button_tag` in the action in order to generate a
button without a form. Then, we could add either a `form` or a
`formaction` attribute to the button.
However, I thik it's easier to move the delete button outside the update
button <form> tag. On the minus side, since the buttons no longer share
a parent, they're harder to style. So we're using a mix of nested flex
layouts with one of the nested elements using a container unit as width.
Since we're at it, we're also improving the styles on small and medium
screens by making sure the "Update" button wraps before the "Delete"
button does (using a container query), by giving enough width to the
column containing this actions on small screens as well (removing
`small-12` and giving it two-thirds of the width on all screen sizes)
and by having a gap between elements.
Note that, at the time of writing, container queries are only supported
by about 91%-93% of the browsers, meaning that some administrators will
see all from controls displayed vertically, one on top of the other, on
all screen sizes. We think this is acceptable, and the page remains
fully functional in this case.
We were already using buttons to destroy pages from the pages index.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
This way we can simplify setting the title and styling the link in the
header. We're also fixing the unnecessary padding introduced by the
`column` classes, which caused the header not to be aligned with the
rest of the elements surrounding it. We're still keeping it the margin
used in the `row` classes so it's aligned with the rest of the form;
ideally, we would remove the `row` classes in the rest of the form and
in the whole admin section, but this isn't something we can tackle right
now.
Note that, in the CSS, the `margin-left: auto` property needs to be
included after `@include regular-button` because that mixin overwrites
the `margin-left` property. Since we're modifying this code, we're
making it compatible with RTL text, using `$global-left` instead of
`left`.
The `use_helpers` method was added in ViewComponent 3.8.0, and it's
included by default in all components since version 3.11.0.
Note we sometimes delegated the `can?` method to the controller instead
of the helpers, for no particularly reason. We're unifying that code as
well.
In the admin section, when clicking on a link that leads to a page in
the public area, sometimes the page was opened in the same window and
sometimes it would open in a new window, with no clear criteria
regarding when either scenario would take place.
This was really confusing, so now we're more consistent and open
(almost) every link in the same window. The main reason behind it is
simple: if we add `target: _blank`, people who want to open those links
in the same window can no longer do so, so we're taking control away
from them. However, if we don't add this attribute, people can choose
whether to open the link on the same tab or to open it on a new one,
since all browsers implement a method to do so.
More reasons behind this decision can be found in "Opening Links in New
Browser Windows and Tabs" [1].
We're keeping some exceptions, though:
* Opening the link to edit an investment on the same tab would result in
losing all the investment filters already applied when searching for
investments, so until we implement a way to keep these filters, we're
also opening the link to edit an investment in a new tab
* For now, we're also opening links to download files in a new window;
we'll deal with this case in the future
[1] https://www.nngroup.com/articles/new-browser-windows-and-tabs/
In some places, we were using `blank` instead of `_blank`. Most browsers
treat `blank` like `_blank`, though, so most people didn't experience
any difference.
In another place, we were incorrectly passing the `target` option inside
an `options:` hash, resulting in invalid HTML.
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.
We weren't showing the year when a page was created/updated, and we
were displaying the created date instead of the updated one.
Co-Authored-By: Diego Calvo <diego.calvo@enreda.coop>
We were executing SQL queries searching for translations even for
records which were not persisted in the database and therefore had no
translations.
So we're now only looking for translations when the record is persisted.
Note that, when the record isn't persisted, we're using `I18n.translate`
instead of the `t` method. That's because the `t` method would also
perform database queries since we overwrite the `t` method in the
`i18n_translation` initializer.
On tabs with many records, like the proposals tab, requests are now up
to three times faster on production environments.
Tests related to this part of the application were failing sometimes
because requests took longer than `Capybara.default_max_wait_time`.
After these changes, the custom information texts pages load about 30%
faster when running the tests.
This way it's easier to test; changing it will also be easier.
During my experiments I made a mistake which wasn't covered by the test
suite. We're adding a test for this case.
Note we're using `i18n_content` in the component instead of `content`
because there's already a `content` method provided by ViewComponent.
