In the past it would have been confusing to add a way to directly
enable/disable a phase in the phases table because it was in the middle
of the form. So we would have had next to each other controls that don't
do anything until the form is sent and controls which modify the
database immediately. That's why we couldn't add the checkboxes we used
when using the wizard.
Now the phases aren't on the same page as the budget form, so we can
edit them independently. We're using a switch, so it's consistent with
the way we enable/disable features. We could have used checkboxes, but
with checkboxes, users expect they aren't changing anything until they
click on a button to send the form, so we'd have to add a button, and it
might be missed since we're going to add "buttons" for headings and
groups to this page which won't send a form but will be links.
Since we're changing the element with JavaScript after an AJAX call, we
need a way to find the button we're changing. The easiest way is adding
an ID attribute to all admin actions buttons/links.
Having links in the middle of a form distracts users from the task of
filling in the form, and following a link before submitting the form
will mean whatever has been filled in is lost.
And the budgets form is already very long and hard to fill in. Having
the phases table in the middle of it made it even harder. And, since
we're planning to add the option to manage groups and headings from the
same page, it's better to have a dedicated page for the form.
In component tests, the `within` method is actually an alias to RSpec's
`be_within` matcher, which is used to test numeric ranges. That meant
the tests always passed, even when there were bugs on the page.
In order to use `within` in component tests, we have to use
`page.within`. However, that also fails, since there's no such method
for `Capybara::Node::Simple'` objects, which are used in component
tests.
So we're using `page.find` instead.
See also pull request 945 in https://github.com/github/view_component
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.
We were using buttons with the "Enable" and "Disable" texts to
enable/disable settings. However, when machine learning settings were
introduced in commit 4d27bbeba, a switch control was introduced to
enable/disable them.
In order to keep the interface consistent, we're now using switch
controls in other sections where settings are enabled/disabled. We can
even use the same code in the machine learning settings as well.
We're also removing the confirmation dialog to enable/disable a setting,
since the dialog is really annoying when changing several settings and
this action can be undone immediately. The only setting which might need
a confirmation is the "Skip user verification" one; we might add it in
the future. Removing the confirmation here doesn't make things worse,
though; the "Are you sure?" confirmation dialog was also pretty useless
and users would most likely blindly accept it.
Note Capybara doesn't support finding a button by its `aria-labelledby`
atrribute. Ideally we'd write `click_button "Participatory budgeting"`
instead of `click_button "Yes"`, since from the user's point of view the
"Yes" or "No" texts aren't button labels but indicators of the status of
the setting. This makes the code a little brittle since tests would pass
even if the element referenced by `aria-labelledby` didn't exist.
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.
We're not adding the rule because it would apply the current line length
rule of 110 characters per line. We still haven't decided whether we'll
keep that rule or make lines shorter so they're easier to read,
particularly when vertically splitting the editor window.
So, for now, I'm applying the rule to lines which are about 90
characters long.
As mentioned in commit 5214d89c8, using a `<select>` tag which
automatically submits a form on change has a few accessibility issues,
particularly for keyboard users who might accidentally submit the form
while browsing the options.
So we're adding a submit button and removing the "submit on change"
behavior.
Note that, while `<select>` tags have their own usability issues,
alternatives in this case are not obvious because the number of existing
polls could be very low (zero, for instance) or very high (dozens, if
the application has been used for years).
I thought of using a `<datalist>` tag with a regular text input. The
problem here is we don't want to send the name of the poll to the server
(as we would with a `<datalist>` tag); we want to send the ID of the
poll.
Maybe we could add an automplete field instead, providing a similar
funcionality. However, for now we're keeping it simple. This poll
questions page isn't even accessible through the admin menu since commit
83e8d603, so right now anything we change here will be pretty much
useless.
The word "budget" in the "Preview budget" link is redundant.
On the other hand, the words "Manage", Edit" and "Admin" are not
really necessary in my humble opinion. Just like in the admin
navigation menu we use "Participatory budgets" instead of "Manage
Participatory budgets", the fact that we're going to manage or
admin or edit something can be deduced from the fact that we're in
the admin section.
Besides, it isn't clear to me why we use "Manage" for projects,
"Edit" for heading groups and "Admin" for ballots. The differences
between these three concepts might be too subtle for me.
The previous paragraphs haven't been corroborated with real users,
though, so I might be mistaken and we might need to revisit these
links in the future.
These actions still take quite a lot of space. Maybe in the future we
could remove the "delete" icon, at least on budgets which cannot be
deleted.
When we see a list of, let's say, banners, and each one has a link to
edit them, the word "banner" in the text "edit banner" is redundant and
adds noise; even for users with cognitive disabilities, it's obvious
that the "edit" link refers to the banner.
In this case, the duration of the budget cannot be determined, and the
application was crashing when trying to do so.
Now we're just returning `nil` as duration.
There's an edge case where the current phase of the budget was disabled.
In this case, the application was crashing.
I'm not sure what we should do regarding this case. Is it OK to allow
disabling the current phase? Is it OK to allow selecting a disabled
phase as the current phase?
Since I'm not sure about it, for now I'm leaving it the same way it was.
Co-authored-by: Julian Herrero <microweb10@gmail.com>
This way we can remove duplication and simplify the code in the view.
Note we're not using the "within" method in the tests to access a row,
because it doesn't seem to work in components tests when passing the
`text:` option.
This partial was going to get too complex since in some places we've got
different texts, different URLs or different confirmation messages.
While we should probably try to be more consistent and that would make
the partial work in most cases, there'll always be some exceptions, and
using a partial (with, perhaps, some helper methods) will become messy
really quickly.
