The planned budget investments redesign includes using icons in some
tables, so we might as well use them everywhere.
The original design used Foundation to show the tooltips. We're using
CSS in order to keep the ERB/HTML code simple. One advantage of using
CSS is we can show the tooltip on focus as well, just like accessibility
guidelines recommend [1]. On the other hand, Foundation tooltips appear
on the sides when the link is at the bottom of the page, making sure
they're visible in this case, while CSS tooltips do not. Neither CSS
tooltips nor Foundation tooltips are dismissable, which might be an
accessibility issue.
Note we aren't changing any ERB files in order to replace links with
icons; we're only changing CSS and one line of Ruby code.
[1] https://www.w3.org/WAI/WCAG21/Understanding/content-on-hover-or-focus
Note the CSS could probably be improved to avoid duplication with other
button style definitions. However, that's fine because we're going to
change the style of the links soon.
For the same reason, I haven't bothered to style every single link the
way it was until now.
In some tables, we had "actions", and some columns were also links
pointing to some places. Having both of them at the same time is
confusing, particularly since traditionally the links in the columns
pointed to the same place as some of the actions (although that's not
the case since commit 48db31cd).
We're still keeping links in tables which don't have an action column.
For instance, the proposals table has a "select" button which would be
harder to use if we had action buttons next to it.
Ruby 2.6 introduces `Enumerable#filter` as an alias to
`Enumerable#select`, and so our Filterable.filter method will not work
with Ruby 2.6.
So we're renaming the method to `filter_by`, which is similar to
`find_by`. We could also change the `filter` method so if a block is
given it delegates to `Enumerable#filter`, the same way ActiveRecord
handles the `select` method, but IMHO this is easier to follow.
Now we can use the checkbox label text directly as locator at spec,
also default web_sections are created before every spec by loading
`db/seeds.rb` file so we were duplicating "Proposals" WebSection.
We were allowing users to check/uncheck the "Visible to valuators"
checkbox even when the budget is finished and so the investments cannot
be edited. So users were still able to check/uncheck this attribute, but
the server was silently rejecting these changes.
We've considered removing the column in this case but decided to keep it
since users can already control which columns they'd like to display.
There are a dozen ways to add an icon used for decoration. Each of them
offers advantages and disadvantages regarding these topics:
* Accessibility
* Ease of use for developers
* Ease of customization for CONSUL installations
* Maintainability
* Resulting file size
* Number of HTTP requests
* Browser support
* Robustness
We were using one of the most common ones: icon fonts. This technique
shines in many of these aspects. However, it misses the most important
one: accessibility. Users who configure their browser to display a
custom font would see "missing character" icons where our icons should
be displayed. Some users have pointed out they use a custom font because
they're dyslexic and webs using icon fonts make it extremely painful for
them [1].
Screen reader users might also be affected, since screen readers might
try to read the UTF-8 character used by the icon (even if it uses a UTF
Private Use Area) and will react to it in inconsistent ways. Since right
now browser support for different techniques to prevent it with CSS
ranges from non-existant (CSS speech module) to limited (use an
alternative text in the `content` property [2]), we've been adding an
HTML element with an `aria-hidden` attribute. However, by doing so the
ease of customizations for CONSUL installations is reduced, since
customizing ERB files is harder than customizing CSS.
Finally, font icons are infamous for not being that robust and
conflicting with UTF settings in certain browsers/devices. Recently Font
Awesome had a bug [3] because they added icons out of the Private Use
Area, and those icons could conflict with other UTF characters.
So, instead of loading Font Awesome icons with a font, we can add them
using their SVG files. There are several ways to do so, and all of them
solve the accessibility and robustness issues we've mentioned, so that
point won't be mentioned from now on.
All these techniques imply having to manually download Font Awesome
icons every time we upgrade Font Awesome, since the `font-awesome-sass`
gem doesn't include the `sprites/` and `svgs/` folders Font Awesome
includes in every release. So, from the maintenance poing of view,
they're all pretty lacking.
Method 1: SVG sprites with inline HTML
We can use SVG files where template icons are defined, like so:
<svg>
<use xlink:href="solid.svg#search"></use>
</svg>
This technique has great browser support and it only generates one HTTP
request for all icons. However, it requires adding <svg> tags in many
views, making it harder to customize for CONSUL installations. For
developers we could reduce the burden by adding a helper for these
icons.
Downloading all the icons just to use one (or a few) might also be
inconvenient, since the total file size of these icons will be up to a
megabyte. To reduce the impact of this issue, we could either minimize
the SVG file, compress it, or generate a file with just the icons we
use. However, generating that custom file would be harder to maintain.
Method 2: CSS with one SVG icon per file
We can use the separate SVG files provided by Font Awesome, like so:
background: url("solid/search.svg");
Or, if we want to add a color to the icon:
backgound: blue;
mask-image: url("solid/search.svg");
Using this technique will result in one HTTP request per icon, which
might affect performance. Browser support is also limited to browsers
supporting mask-image, which at the time of writing is 95% of the
browsers, with the notable exception of Internet Explorer 11.
On the plus side, using CSS makes it easy to customize and (IMHO) easy
to work with on a daily basis.
Method 3: CSS with SVG sprites
We can use the aforementioned sprites provided by Font Awesome and use
them with CSS:
backgound: blue;
mask-image: url("solid.svg#search");
The number of HTTP requests and file size are similar to Method 1, while
browser support, ease of customization and ease of use are similar to
Method 2.
There's one extra gotcha: this method requires doing minor changes to
the files provided by Font Awesome, which means this solution is harder
to maintain, since we'll have to do the same changes every time we
upgrade Font Awesome. Mainly we need to add these changes to every
sprite file:
- <svg xmlns="http://www.w3.org/2000/svg" style="display: none;">
+<!--
+This is a modified version of Font Awesome Free regular sprite file.
The icons are exactly as they originally were; the only changes are:
+
+* <symbol> tags have been replaced with <svg> tags and a <style> tag
has been added
+* A <style> tag has been added
+* The style="display:none" attribute of the main <svg> tag has been
removed
+-->
+<svg xmlns="http://www.w3.org/2000/svg">
+ <style>
+ svg svg { display: none }
+ svg svg:target { display: inline }
+ </style>
And then replace every <symbol> tag with a <svg> tag.
Method 4: CSS with Data URI
Finally, we can write the icons directly in the CSS:
backgound: blue;
mask-image: url('data:image/svg+xml;utf8,<svg...');
This method does not generate any extra HTTP requests and only downloads
the icons we need. However, maintaining it is really hard, since we need
to manually copy all the <svg> code for every icon we use, and do it
again every time we upgrade Font Awesome.
In this commit, we implement Method 2. To improve browser support, we're
falling back to font icons on browsers which don't support mask images.
So 5% of the browsers might still conflict with users changing the fonts
or with screen readers trying to announce the icon character. We believe
this is acceptable; the other option for these browsers would be to show
those icons as a background image, meaning the icons would always be
black, meaning users of these browsers would have trouble to distinguish
them if the background was dark as well.
Since we aren't sure whether the performance hit of having one HTTP
request per icon is overcome by only requesting the icons we actually
use, we aren't taking this factor into account when choosing between
methods 2 and 3. We believe this method will be the less painful one to
maintain and customize. Generating SVG sprites with just the icons we
use would increase performance, but it would make it harder for existing
CONSUL installations to use icons we haven't included in the sprites.
[1] https://speakerdeck.com/ninjanails/death-to-icon-fonts
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/content#Browser_compatibility
[3] https://blog.fontawesome.com/fixing-a-unicode-bug-in-5-14-0/
We were using `display: inline` assuming that's the way elements will be
displayed by default when shown. However, those elements could be
displayed in a different way (inline-flex, for instance). So we avoid
any possible conflicts by using the `display: none` rule when we want to
hide the elements.
Besides, the code is now symmetrical and IMHO easier to follow.
Even if "r" is shorter, "regular" is easier to understand, and we're
going to store these icons in a folder named "regular", which is the
convention Font Awesome uses.
After upgrading to Turbolinks 5, redirects are followed on AJAX
requests, so we were accidentally redirecting the user after they mark
an investment as visible to valuators.
There was already a system spec failing due to this issue ("Admin budget
investments Mark as visible to valuators Keeps the valuation tags");
however, it only failed in some cases, so we're adding additional tests.
Ideally we would write a system test to check what happens when users
click on the checkbox. However, from the user's point of view, nothing
happens when they do so, and so testing it is hard. There's a usability
issue here (no feedback is provided to the user indicating the
investment is actually updated when they click on the checkbox and so
they might look for a button to send the form), which also results in a
feature which is difficult to test.
So we're writing two tests instead: one checking the controller does not
redirect when using a JSON request, and one checking the form submits a
JSON request.
I've chosen JSON over AJAX because usually requests to the update action
come from the edit form, and we might change the edit form to send an
AJAX request (and, in this case, Turbolinks would handle the redirect as
mentioned above).
Another option would be to send an AJAX request to a different action,
like it's done for the toggle selection action. I don't have a strong
preference for either option, so I'm leaving it the way it was. At some
point we should change the user interface, though; right now in the same
row there are two actions doing basically the same thing (toggling
valuator visibility and toggling selection) but with very different user
interfaces (one is a checkbox and the other one a link changing its
style depending on the state), resulting in a confusing interface.
We removed it in commit d639cd58 because it recommended using `uniq`
where `distinct` was more appropriate. This has been fixed in
rubocop-rails 2.6.0.
This rule was added in Rubocop 0.89.0. However, there are some false
positives when we don't use interpolation but simply concatenate in
order to avoid long lines. Even if there weren't false positives, there
are places where we concatenate to emphasize the point that we're adding
a certain character to a text.
We might reconsider this rule in the future, since we generally prefer
interpolation over concatenation.
There were places where we had two links pointing to the same place; one
link would be the name/title of a record, and one link would be under
the "actions" column.
This is confusing, since users would probably expect these links to
point to different places (which is what happens in other tables in the
admin section) and might try to click one of them and then the other
one and be surprised when they found out both of them go to the same
page.
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.
In the past we were using some <div> tags surrounding table action
links in order to guarantee these links wouldn't be wider that their
cell's space and wouldn't expand over two lines.
However, while these links didn't expand over two lines, at certain
resolutions the width of their text exceeded the width of the links,
causing part of the text to be outside their borders.
This behavior was also inconsistent: some tables had these <div> tags,
and some tables didn't.
Since we've now introduced the table actions component, the code is more
consistent and we're getting rid of these <div> tags. So now we're again
facing the issue where links could expand over two lines.
Using a flex layout solves this issue and considerably improves the
layout at lower resolutions.
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.
While Rails provides a lot of functionality by default, there's one
missing piece which is present in frameworks like Django or Phoenix: the
so-called "view models", or "components".
It isn't easy to extract methods in a standard Rails view/partial, since
extracting them to a helper will make them available to all views, and
so two helper methods can't have the same name. It's also hard to
organize the code in modules, and due to that it's hard to figure out
where a certain helper method is supposed to be called from.
Furthermore, object-oriented techniques like inheritance can't be
applied, and so in CONSUL customizing views is harder that customizing
models.
Components fix all these issues, and work the way Ruby objects usually
do.
Components are also a pattern whose popularity has increased a lot in
the last few years, with JavaScript frameworks like React using them
heavily. While React's components aren't exactly the same as the
components we're going to use, the concept is really similar.
I've always liked the idea of components. However, there wasn't a stable
gem we could safely use. The most popular gem (cells) hasn't been
maintained for years, and we have to be very careful choosing which gems
CONSUL should depend on.
The view_component gem is maintained by GitHub, which is as a guarantee
of future maintenance as it can be (not counting the Rails core team),
and its usage started growing after RailsConf 2019. While that's
certainly not a huge amount of time, it's not that we're using an
experimental gem either.
There's currently a conflict between view_component and wicked_pdf.
We're adding a monkey-patch with the fix until it's merged in
wicked_pdf.
We can find the booth through the booth assignment, so we don't need to
pass it in the URL.
Since the parameter is in the URL and not sent through a form, we can
also use `params[:poll_id]` directly, and so we can reuse the
`load_poll` method.
We were manually adding forgery protection to all our controllers, but
in Rails 5.2 there's an option (enabled by default for new applications)
which adds this protection to all controllers.
Rails 5.2 is raising a warning in some places:
DEPRECATION WARNING: Dangerous query method (method whose arguments are
used as raw SQL) called with non-attribute argument(s). Non-attribute
arguments will be disallowed in Rails 6.0. This method should not be
called with user-provided values, such as request parameters or model
attributes. Known-safe values can be passed by wrapping them in
Arel.sql().
IMHO this warning is simply wrong, since we're using known PostgreSQL
functions like LOWER() or RANDOM(). AFAIK this code works without warnings
in Rails 6.0 [1][2]
However, since the warning is annoying, we need to take measures so our
logs are clean.
[1] https://github.com/rails/rails/commit/6c82b6c99d
[2] https://github.com/rails/rails/commit/64d8c54e16
Instead of having a header with a bottom margin followed by an element
with a negative margin, it makes more sense to have no margin on either
element.
On high-resolution screens where neither the menu nor the main content
were filling the screen, there was a blank space at the bottom which
looked weird.
The <main> tag was including the navigation, and now we use the same
flex layout, making it more accessible for mobile phone users.
I'm not sure the <main> tag should actually include the account info and
the flash message. I'm keeping it like this in order to keep the layout
the way it was.
While Foundations's off-canvas menu allows us to forget about writing
CSS, it also leads to complicated HTML.
Ideally Foundation would provide an easy way to simplify what we're
doing, but I haven't found anything in the documentation.
We could simplify the HTML a bit more if we used a CSS grid layout
instead of a flex one, but old browsers have better support for the
latter.
Note we're using `breakpoint(medium)` so we can group the CSS for small
screens and follow SCSS-Lint rules at the same time.
Also note behavior of the main area when the menu appears on small
screens is slightly different: it doesn't move the main content to the
right. I've done it this way so we don't have any overflow issues,
unlike the previous version.
There's a small issue using a label and a checkbox to enable/disable the
menu: sighted keyboard users with a small screen might not be able to
enable the menu. So we're adding the `:focus-within` pseudoclass so the
menu can be normally navigated using the keyboard. Even if old browsers
don't support this pseudoclass, we believe the probability of a sighted
user using a small screen, navigating with the keyboard and using an old
browser is really low, particularly in the admin area.
We're also adding the `aria-hidden` attribute on the label, since the
menu is never hidden for screen readers and so having a control to show
it could be confusing. Since the label is not focusable, we're complying
with the fourth ARIA rule:
> Do not use role="presentation" or aria-hidden="true" on a focusable
> element .
>
> Using either of these on a focusable element will result in some users
> focusing on 'nothing'.
