Using placeholders having similar (or identical) text as already present
as a label has a few issues.
First, it's a distraction. Reading the same information twice is
useless, requires an extra effort, and might even frustrate users.
Second, if users start typing before reading the placeholder and see it
disappear, they might think they're missing relevant information,
delete what they typed, and read the placeholder. That will get them
nowhere.
Finally, we display placeholders using a text offering very low contrast
against the background, so users don't think the placeholder is an
actual value entered in the field. Using such low contrast makes the
text hard to read, particularly for users with visual impairments.
So we're removing these placeholders.
This commit only deals with placeholder texts with similar (or
identical) texts as the label text. There might be other places where we
should replace placeholder texts with labels, but that's a different
topic.
This test will fail if the ID of the created record is the same as the
ID of the first user we create in the test. The chance is very low due
to the `rand(9999999)` which causes the test to fail just once every ten
million times. However, why assigning the ID in the first place? Without
it, the test will never fail due to conflicting IDs.
When administrators disabled features and users tried to access them
with the browser, we were responding with a 500 "Internal Server Error"
page, which in my humble opinion was incorrect. There was no error at
all; the server worked exactly as expected.
I think a 403 "Forbidden" code is better; since that feature is
disabled, we're refusing to let users access it.
We could also respond with a 404 "Not found", although I wonder whether
that'll be confusing when administrators temporarily or accidentally
disable a feature.
A similar thing might happen if we responded with a 410 "Gone" code.
Actually this case might be more confusing since users aren't that
familiar with this code.
In any case, all these options are better than the 500 error.
Note we don't cast negative votes when users remove their support. That
way we provide compatibility for institutions who have implemented real
negative votes (in case there are / will be any), and we also keep the
database meaningful: it's not that users downvoted something; they
simply removed their upvote.
Co-Authored-By: Javi Martín <javim@elretirao.net>
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
It was hard to notice what was going on when supporting one investment
which was at the bottom of the investment index page.
I wonder whether we should add the title of the investment to this text;
I'm not doing so because we don't do that anywhere else.
Using links combined with JavaScript to generate POST requests to the
browser has a few issues.
An obvious one is that it doesn't work for users without JavaScript
enabled (which lately I've noticed are more common than I thought, even
though I've been one of them for years). These users will reach a 404
page.
Since CONSUL isn't currently designed to work without JavaScript
enabled, let's ignore this one for now.
A not so obvious issue is screen reader users might expect the link to
take them somewhere instead of performing an action (in this case,
sending a form to the server).
There might be more issues I'm unaware of. Quoting DHH [1]:
"Turning ahrefs into POSTs is a bit of an anti-pattern, especially for
a11y reasons. Better to use button_to with a styling."
So we're using a button instead. This way we can also simplify the code
and make the button disabled for unidentified users, which automatically
makes it impossible to focus it using the keyboard.
A possible disadvantage of using `button_to` is it will create a form
tag which will be announced to screen readers as a form landmark. I've
tested it with my screen reader and everything worked fine for me, but
some screen reader users might interact with these forms in a different
way and find it confusing, particularly in the case where the button is
disabled.
With this change, we're only changing links for buttons in one place.
There are other places where we should do similar changes.
[1] See issue 33 in https://github.com/hotwired/turbo-rails/
We meant to check that the link to support a different investment didn't
have an alert dialog. However, we were testing by trying to support the
same investment twice, and here there was no link at all, so the
assertion "there's no link with a dialog" always passed even when the
application didn't behave properly.
Now we check there's a link there, and that link doesn't have an
associated alert dialog.
In regular system tests, we prefer testing against the text of the link
because it's easier to recognize which link we're talking about.
I was wondering whether we should remove the `title` attribute
completely and use just the `aria-label` attribute. Quoting The Paciello
Group blog [1]:
"If you want to hide content from mobile and tablet users as well as
assistive tech users and keyboard only users, use the title attribute."
However, there's a case where mouse users might find the title attribute
useful. When visiting an investment page, the connection between the
"Support" link and the investment is not as clear as it is in the
investments index page, so it might not be clear what you're supporting.
As mentioned, though, this information will only be relevant for mouse
users, who nowadays represent less than half of our users.
[1] https://www.tpgi.com/using-the-html-title-attribute-updated/
The `js-` prefix (which I admit I'm not fond of) is usually used to
indicate the class is used by JavaScript files, not for using it in test
files. And in all the other similar tests, we're using the `in-favor`
class instead of the `js-in-favor` class.
This reverts commit 83fe74d53.
When supporting an investment in the management section through the
investment view, we were accessing an action in the public investments
controller. This meant the manager was the one supporting the investment
(as they'd be the `current_user` in this controller) and not the managed
user.
In the case of groups with many headings, voting the first time requires
a confirmation and then a regular (non-AJAX) request takes place. In
this case, users were redirected to the public area instead of remaining
in the management area.
Using the proper URL to vote solves the problem.
Note there was a comment about one of these tests failing in Travis.
Most probably the test failed because there was no expectation after
clicking the link with the investment title, so the "Support" button
(which is also present in the index page) was clicked before the
investment view was loaded.
The test was failing sometimes because the conditions we were checking
were the same before and after clicking the "Pending" link. So there was
a race condition between the request generated by clicking the "Pending"
link and the order to click the "Confirm moderation" link. This
sometimes resulted in the "Confirm moderation" link not being correctly
clicked.
The key was declared twice and so the first one ("Support this project")
was overwritten.
We're grouping all keys related to the investment list together in order
to reduce the chances of this issue happening again (or, at least, in
this part of the code).
Note one of the tests dealing with random results is a bit flaky; since
it's a permutation selecting 7 objects out of 12, it will fail about
once every 4 million times. We think this is acceptable.
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
In the form of creating a new investment was hiding the name of the
group if it had only one heading, but could be confusing to users if
there are, for example, five different groups of one heading.
The solution:
- If the budget has one group and one heading, the heading selector is
hidden.
- If the budget has one group and more than one heading, the group name
is hidden.
- If the budget has more than one group, the group name appears
regardless of the number of headings.
Note we're using an extra `<span>` element but we could use a CSS grid
layout instead. We're not using it because browser compatibility is only
94.56% at the time of writing.
- Allow to define a link (text and url) on budget form for render on the budget
header.
- Improve styles
Co-authored-by: Senén Rodero Rodríguez <senenrodero@gmail.com>
When users created a budget and made a typo, they could use the link to
go back to edit a budget. However, after doing so, they were out of the
budget creation process.
So we're now letting users go back to edit the budget, fix any mistakes
they might have made, and then continue to groups.
So now there's no need to edit each phase individually to enable/disable
them.
We aren't doing the same thing in the form to edit a budget because we
aren't sure about possible usability issues. On one hand, in some tables
we automatically update records when we mark a checkbox, so users might
expect that. On the other hand, having a checkbox in the middle of a
form which updates the database automatically is counter-intuitive,
particularly when right below that table there are other checkboxes
which don't update the database until the form is submitted.
So, either way, chances are users would think they've updated the phases
(or kept them intact) while the opposite would be true.
In the form within the wizard to create a budget that problem isn't that
important because there aren't any other fields in the form and it's
pretty intuitive that what users do will have no effect until they press
the "Finish" button.
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
Note we're keeping this section's original design (which had one button
to add a new group which after being pressed was replaced by a button to
cancel) but we aren't using Foundation's `data-toggle` because there
were a couple of usability and accessibility issues.
First, using `data-toggle` multiple times and applying it to multiple
elements led to the "cancel" button not being available after submitting
a form with errors. Fixing it made the code more complicated.
Second, the "Add new group" button always had the `aria-expanded`
attribute set to "true", so my screen reader was announcing the button
as expanded even when it wasn't. I didn't manage to fix it using
`data-toggle`.
Finally, after pressing either the "Add new group" and "Cancel" buttons,
the keyboard focus was lost since the elements disappeared.
So we're simplifying the HTML and adding some custom JavaScript to be
able to handle the focus and manually setting the `aria-expanded`
attribute.
Co-Authored-By: Javi Martín <javim@elretirao.net>
Co-Authored-By: Julian Herrero <microweb10@gmail.com>
We don't allow deleting a budget with associated investments. However,
we allow deleting a budget with associated administrators and valuators.
This results in a foreign key violation error:
PG::ForeignKeyViolation: ERROR: update or delete on table "budgets"
violates foreign key constraint "fk_rails_c847a52b1d" on table
"budget_administrators"
Using the `dependent: :destroy` option when defining the relationship,
we remove the association records when removing the budget.
As a bonus, we reduce the number of Rubocop offenses regarding the
`Rails/HasManyOrHasOneDependent` rule. Only 72 to go! :)
Some users might not be able to touch the icon due to a motor
disability. Other users might think the "Menu" text is part of the
button and try to touch it instead.
Making the "Menu" text part of the button makes it easier to show/hide
this menu. Besides, it lets screen reader users with a small screen hear
the word "Menu" associated to the button.
We could simplify the HTML a bit more but Foundation's `hamburger` mixin
uses the `::after` element with `position: absolute`, so we can't apply
it directly to the button without making the CSS more complex.
