Sometimes tests were hanging indefinitely. Debugging shows that in some
cases it's due to submitting a form before the AJAX request to get
proposals, debates or investments suggestions is finished, since having
an AJAX and a non-AJAX request at the same time when running the test
sometimes leads to unexpected results.
In our case, we were having many timeouts in Github Actions in the
branches where we use both ActiveStorage and Paperclip to store files
(based on pull request 4598). I can reproduce it in those branches
running the following test ("Should show new image after successful
creation with one uploaded file"), although only when my laptop isn't
plugged (!!):
```
rspec './spec/system/proposals_spec.rb[1:33:1:14]'
```
Since we didn't have a proper way to know the AJAX request had finished,
we're adding a `suggest-success` class to the element showing the
suggestions when that happens. Then in the tests we can look for that
class after filling in the title of a proposal, debate or investments.
Just for clarity's sake, we're also adding the `suggest-loading` class
when the suggestions are loading.
In order not to have expectations everywhere about the suggestions,
we're extracting methods to fill in those titles in the tests. Note we
aren't using these methods in the "edit" actions (suggestions are not
showing when editing) or in tests with the `no_js` tag (since
suggestions only work with JavaScript).
There were times when some actions had text expanding over two lines
while other actions had text on just one line.
Since all the items in a row had the same height, the elements with just
one line added a blank second line. This was obvious when hovering with
the mouse or focusing on the element with the keyboard. It looked weird
when the element received focused, and it could also lead to
unintentional clicks when clicking on the blank space below the element.
The underline text decoration looked a bit strange with the icon on top
of it, and we had an inconsistency since links had a text decoration but
buttons didn't. I think we can remove it since both the color of the
icon and the cursor change on hover and that's enough feedback.
And we were accidentally overwriting the color of the investments link
on hover and focus.
By default, Rails disables submit inputs (<input type="submit">) when
they're pressed so we avoid a double-submission when users click the
button twice.
However, Rails does not disable submit buttons (<button type="submit">)
when they're pressed. This means there's a chance users might press the
button several times. Even if most our table actions are idempotent, it
might cause certain issues. For instance, pressing the "Delete" button
twice means the second request might raise an
`ActiveRecord::RecordNotFound` exception.
Disabling the button also gives feedback to users, letting them know
they've correctly clicked the button.
Links acting like buttons have a few disadvantages.
First, screen readers will announce them as "links". Screen reader users
usually associate links with "things that get you somewhere" and buttons
with "things that perform an action". So when something like "Delete,
link" is announced, they'll probably think this is a link which will
take them to another page where they can delete a record.
Furthermore, the URL of the link for the "destroy" action might be the
same as the URL for the "show" action (only one is accessed with a
DELETE request and the other one with a GET request). That means screen
readers could announce the link like "Delete, visited link", which is
very confusing.
They also won't work when opening links in a new tab, since opening
links in a new tab always results in a GET request to the URL the link
points to.
Finally, submit buttons work without JavaScript enabled, so they'll work
even if the JavaScript in the page hasn't loaded (for whatever reason).
For all these reasons (and probably many more), using a button to send
forms is IMHO superior to using links.
There's one disadvantage, though. Using `button_to` we create a <form>
tag, which means we'll generate invalid HTML if the table is inside
another form. If we run into this issue, we need to use `button_tag`
with a `form` attribute and then generate a form somewhere else inside
the HTML (with `content_for`).
Note we're using `button_to` with a block so it generates a <button>
tag. Using it in a different way the text would result in an <input />
tag, and input elements can't have pseudocontent added via CSS.
The following code could be a starting point to use the `button_tag`
with a `form` attribute. One advantage of this approach is screen
readers wouldn't announce "leaving form" while navigating through these
buttons. However, it doesn't work in Internet Explorer.
```
ERB:
<% content_for(:hidden_content, form_tag(path, form_options) {}) %>
<%= button_tag text, button_options %>
Ruby:
def form_id
path.gsub("/", "_")
end
def form_options
{ id: form_id, method: options[:method] }
end
def button_options
html_options.except(:method).merge(form: form_id)
end
Layout:
<%= content_for :hidden_content %> # Right before the `</body>`
```
The message "Are you sure?" is usually followed by blindly clicking
"Yes" without really thinking about what one is doing. So we're
including a bit more information about what's about to happen. That way
it's more likely users will notice it when they accidentally click the
wrong button.
Ideally we would offer the option to undo every common action and then
we wouldn't have to ask for confirmation. But since that isn't the case,
for now we're adding a better confirmation message.
Note we're removing the `resource_name` parameter from the translation
to confirm the action of deleting a record. The reason is, in many
languages it only makes sense to add the model name when it's got an
associated article, and, unlike in English (where "the" is used for
every word), that article is different depending on the noun it's
related to. So we'd have to provide a translation like "name with
article, when singular" for every model.
The complexity of these translations could scalate quickly. And, given
the context, IMHO it isn't essential to add the resouce name. When we're
in the proposals index and there's a proposal named "Improve XYZ", and
we click on "Delete" and see a message saying "This action will delete
XYZ", it is implied that XYZ is a proposal.
So instead we're changing the message so it works for every record with
no need of noun-dependent articles.
This way screen reader users will know which record they're going to
access when focusing on a link to a certain action. Otherwise they'd
hear something like "Edit, link", and they wouldn't know which record
they'll end up editing if they follow the link.
This way it will be easier to change the behavior of all table actions,
like adding ARIA attributes. In the past, when we changed the behavior
of the `link_to` method, we had to change all table action classes.
Styles on keyboard focus are essential for keyboard navigation; without
them, keyboard users wouldn't see which element they're currently
interacting with. That's why we use an `outline` on elements having the
current keyboard focus.
However, this is sometimes annoying for mouse/touchscreen users, since
clicking/touching an element also gives it keyboard focus.
When clicking on a button performing some kind of action through
JavaScript, keeping the outline on the button after clicking it is
distracting.
Even after clicking a link, for some users having the outline present
while they wait for the next page to load is annoying.
That's why modern browsers (at the time of writing, 74%) implement the
`:focus-visible` pseudoclass, which selects elements which have received
focus using the keyboard, but not elements which have been
clicked/tapped on. We can use it to provide focus styles for keyboard
users without getting in the way of mouse/touchscreen users.
Usually we wouldn't use a feature which isn't supported in more than 96%
of the browsers out there. However, in this case we've got a solid
fallback: we just use the `:focus` pseudoclass. Since the `@support` CSS
condition doesn't accept pseudoclasses as parameters, we're disabling
`:focus` styles only on browsers supporting the `:focus-visible`
pseudoclass using the `:focus:not(:focus-visible)` selector, which will
be ignored by browsers without support for `:focus-visible`.
Since now users receive less feedback when clicking/touching a link or a
button, we're adding styles for the `:active` pseudoclass. This way
users will know which item they're clicking/tapping on. I'm not sure the
outline is a good option for this case, though; I think for touchscreen
users a better solution would be to apply the styles we apply on hover.
We might change it in the future.
Note grouping styles together like this would *not* work:
```
&:focus,
&:focus-visible {
// Styles here
}
```
Browsers not supporting the `:focus-visible` pseudoclass would ignore
this statement completely, meaning they wouldn't apply the styles on
`:focus` either.
We lost focus styles on certain controls in commit 4dad04ae3, since we
were applying a border with a rule which had more precedence than the
rule of border on focus.
We were using a focus outline on links, but weren't doing the same for
buttons. Since sometimes browsers use a default outline which is barely
visible, this was very disorienting when browsing using the keyboard; we
were navigating through links that clearly indicated where the keyboard
focus was, and when reaching a button suddenly we had this almost
imperceptible feedback. Even if I'm used to it, my first reaction is
always "where did the focus go?" until I realize it's now on a button.
This is even more confusing because we've got buttons looking like links
and links looking like buttons.
Note that in the rules for the `:focus` styles we're including buttons
and the `[type="button"]` attribute. This seems redundant since those
styles are already covered by the `button` selector. However, Foundation
adds styles to buttons with the `[type]` attribute. Since the attribute
selector has precedence over the tag selector, we need to use the
attribute selector as well in order to override Foundation's styles.
There were no elements matching this selector since commit d679c1eb7 and
these styles were completely ignored. I'm re-adding the ones with make
sense in my humble opinion. I'm not adding top and bottom paddings since
they affect the way the height of the element is calculated, and am not
sure about the intention behind setting the height property.
The "Sign in" or "My account" links and the main navigation are
different elements, and they're in different places on medium and large
screens. Now we're also separating them on small screens.
Since the `.vertical` class in the menu added quite a few styles and it
was difficult to overwrite them, we're simply removing this class from
this element. This way we're also removing the huge space between the
menu button and the first element of the navigation.
On small screens, the "Sign in", "Register", "My content", "My account"
and "Sign out" links didn't have much padding nor space between them,
and it was easy to accidentally click the wrong link.
This change also positively affects the menu on medium and large
screens. When one of the options (like "SDG content") had a text
spanning over two lines (like it happens in Swedish), there was barely
any space between those two lines. So we're using `line-height: inherit`
instead and adjusting the padding accordingly.
It was broken since we fixed issues with other image fields in commit
394a94cbf, because there we added a fieldset for the image fields, and
so they inherited the styles in budget phases fields (these styles were
added before it was possible to attach an image to a phase).
In commit 55c3c7987 we updated the admin layout header to use
`class="dropdown menu" data-dropdown-menu` instead of `class="menu"
data-responsive-menu="medium-dropdown"`.
Then, in commit dcec003d0, we updated the public layout header to use
`class="menu" data-responsive-menu="medium-dropdown"` instead of
`class="dropdown menu" data-dropdown-menu`.
Now we're applying the same classes to both, hoping we can get some
consistent styles.
I'm choosing to keep the ones in the public layout because using it I
had less problems when trying to improve the styles of this navigation.
When, on small screens, a navigation element had a very long text
causing it to span over multiple lines, the space between each line was
the same as the space between elements. This made it hard to see where
elements started and ended.
Using a padding to separate the contents of one element and the contents
of the next one solves the issue.
When users see a label saying "With the text" and an input field, they
don't usually need a placeholder saying "Write the text". On the
contrary, this text adds noise and is hard to read due to the low
contrast between the color of the placeholder and the color of the
field, making the text an unnecessary distraction.
User testing has shown this filter isn't really useful and sometimes
makes users wonder what it's about. This is particularly true in CONSUL
installations which don't change the default values (most of them),
since users will see a filter with options like "Official position 1".
We were using helper methods inside the model; we might as well include
them in the model and use them from anywhere else.
Note we're using a different logic for images and documents methods.
That's because for images the logic was defined in the helper methods,
but for documents the logic is defined in the Documentable concern. In
the past, different documentable classes allowed different content
types, while imageable classes have always allowed the same content
types.
I'm not sure which method is better; for now, I'm leaving it the way it
was (except for the fact that we're removing the helper methods).
The same way it's done for images.
We were converting the number of megabytes to bytes and then converting
it to megabytes again. Instead, we can leave it as it is and only
convert it to bytes when necessary (only one place).
The `sign_in(nil)` method was a bit hard to understand IMHO. After all,
in controller and system tests we don't have to specify no user is
signed in; that's the default behavior.
So we're making it the default behavior for component tests as well.
