We need to use page body event delegation so it will work with any
element even with the ones added through ajax, in this case the
annotation comments box form. By doing this way we do not need
this code on the server response anymore.
Furthermore JS events defined at ajax responses are not part of
application javascript and are lost when restoring a page from
browser cache, you can try to apply the same event delegation
technique to the `erb` file and it wont work just because events
added dinamically are not treated the same than `application.js`
code.
To reproduce the error:
1. Load an annotatable draft version
2. Move to any other page
3. Go back
Now "Publish comment" button wont work.
If we do not destroy annotator app before storing the page at
browser cache we will unnecesarily initialize annotations twice (or
more) duplicating Annotator HTML markup and causing
unexpected errors.
Without this commit you will find an error when restoring a page with
annotator, you can click on any annotation and you will see the annotation
comments are being loaded twice.
IMO this is an idempotency issue within Annotator JS library.
Patch extracted from here the comments on turbolinks issue 253 and
converted to vanilla javascript.
The hide action over datepickers ensures us that opened datepickers
will be closed before leving the page. Previously if you open any
datepicker and then move to previous page you will keep seeing the
datepicker in the restored page.
Sometimes we define URLs for POST requests which are not defined for GET
requests, such as "/residence", so redirecting to it after signing out
results in a routing error.
So instead of using the request referer, we're using the stored location
devise uses, and we're not storing locations in POST requests.
For reasons I'm not sure about, the homepage (and the welcome pages)
were an exception in our "redirect users to the same page they were"
policy.
I'm not sure about the welcome pages (no test was present indicating
they should behave in a special way), but in the case of the home page,
it was a bit annoying to be redirected to a different place after
signing in.
When signing in from a page containing GET params, like
`/budgets/1/investments?heading_id=4`, we were redirected to a URL
without those GET params; in this case, `/budgets/1/investments`.
Using the request fullpath, as recommended in the devise documentation,
keeps these parameters when redirecting.
Note we're using a new sanitizer. Ideally we'd reuse the
`AdminWYSIWYGSanitizer`, but then code that would be correctly shown by
markdown-it (like the <h1> tag) wouldn't be shown on the web, which is
confusing. Ideally we would configure markdown-it to only allow the tags
present in the `AdminWYSIWYGSanitizer` and provide some kind of help
showing which tags are allowed.
When skipping verification, we cannot apply the validation rule saying
the document number and document type must be unique, because they'll be
`nil` in many cases. So we were skipping the rule, but that makes it
possible for the same user to vote several times (for instance, once in
a booth and once via web).
So we're changing the scope of the uniqueness rule: instead of being
unique per document number, voters are unique per user. The reason we
made them unique per document number was that back in commit 900563e3
(when we added the rule), we hadn't added the relation between users and
poll voters yet.
Up until now, we were assuming the voter was valid, but were not raising
an exception if it wasn't. And in the user interface everything seemed
to be working properly.
We were having this issue when skipping verification, when there could
be voters without a document number, which would be considered invalid.
Raising an exception when failing to save the voter and making sure the
answer and the voter are saved inside a transaction solves the problem.
Turbolinks 5 handles page caching differently; it saves the current HTML
and, when the page is restored using the browser's back button, it
copies it and triggers the `turbolinks:load` event, which, in our case,
triggers our `initialize_modules` function.
This isn't a problem regarding event handlers, since they're removed
when caching the page (except fot the ones attached to the `document`).
However, it is a problem for functions that, once the document is ready,
scan the DOM and add certain elements. In this case, an element might be
added a second time when the page is restored.
So we need to either check an item has already been added before adding
it or remove it before caching the page. Since this is going to be a
common pattern, we're adding a function to handle these non-idempotent
parts of the application, so it mirrors our `initialize_modules`
function.
We're also moving the `destroy` function definition after the
`initialize` function definition, which makes more sense since we
initialize things before destroying them.
This is the reason why this feature was implemented in the first
place: it's easy to open the editor, make some changes, close it, and
continue without realizing the changes have not been saved.
In the rest of the forms, this functionality is quite lacking. For
starters, some forms warn if there are unsaved changes, while some forms
don't, which is highly inconsistent and disorients users.
Furthermore, we were having problems with this feature after upgrading
Turbolinks, particularly in forms using CKEditor. In these cases, a lot
of hacking needs to be done in order to make this feature work properly,
since CKEditor adds some formatting automatically, and if this is done
after the form is serialized, we'll get some unexpected behavior. On the
other hand, comparing the value of a textarea against its `defaultValue`
property will work on every edge case, including using the browser's
back button or reloading the page.
Finally, users are used to the way web forms work, and aren't used to be
asked for confirmation when they change their mind and decide to leave
the page without saving the changes. Asking them for confirmation will
be annoying in most cases. Besides that, if they accidentally leave the
page, they can use the browser's back button and they'll recover the
unsaved changes.
It's true this won't happen it they accidentally close the browser's
window, but our WatchFormChanges functionality didn't work in this case
either. Using the "beforeunload" event adds more problems than it
solves, since it doesn't support custom messages (or, to be more
precise, modern browsers ignore custom messages), and it doesn't get
along with turbolinks.
Co-Authored-By: Senén Rodero Rodríguez <senenrodero@gmail.com>
Turbolinks 5 doesn't follow the browser's standard behaviour of ignoring
links pointing to "#", so we're preventing the turbolinks events in this
situation:
We didn't upgrade Turbolinks when we upgraded to Rails 5 so we didn't
upgrade too many things at the same time, and postponed it... until now
:).
Note upgrading Turbolinks fixes an issue with foundation's sticky when
using the browser's back and forward buttons. We're adding tests for
these scenarios.
Co-authored-by: Senén Rodero Rodríguez <senenrodero@gmail.com>
The `$()` function is a shortcut for `$(document).ready()`, and we were
attaching events to `$(document).ready()` inside the `$()` function.
In a similar way, we were handling the `page:load` and `ajax:complete`
events inside the `$()` function. But when those events trigger, the DOM
is already ready.
Besides, we don't have to wait for the DOM to be ready before attaching
events to the `document` element. Quoting jQuery's `.on()`
documentation:
> The document element is available in the head of the document before
> loading any other HTML, so it is safe to attach events there without
> waiting for the document to be ready.
Co-Authored-By: Senén Rodero Rodríguez <senenrodero@gmail.com>
In the past we had huge problems trying to make it work with Turbolinks.
However, after updating foundation-rails in commit 58071fd6, these hacks
aren't necessary anymore.
We're adding a test for the scenario of visiting a page using
Turbolinks, which was missing, so we're sure we aren't breaking
anything.
Note the sticky will still not work after using the browser back button.
We haven't been able to make it work with turbolinks-classic; we'll fix
this issue when upgrading turbolinks.
Its known that initializing a map when it is inside a hidden element
wont work when hidden element is shown, so its makes sense to
avoid initialization of hidden maps.
When a map lives within a hidden layer we need to initialize the
map after the event of showing that hidden layer, in our case when
admin settings tab is shown.
TableSortable and Sortable javascripts were using the same CSS
class name to define completely different and separated
behaviours causing unexpected errors. Now `sortable.js` script
will use `.sortable` class and `table_sortable.js` will use
`.table-sortable` instead.
We were displaying two progress bars for the same thing, and hiding one
of them.
Displaying just one of them and readjusting the styles accordingly is a
bit more intuitive IMHO.
We're also getting the text inside the progress bar out of it; its
purpose inside an element with the `progressbar` role is to provide the
same information as the progress bar (which we aren't exactly doing,
although it could be argued that we do), and in order to be accessible
we should provide the same text in the `aria-valuetext` field, which we
aren't doing. This also simplifies our CSS, which was working because we
defined a padding which covered the height of the hidden extra progress
bar and would have needed quite a few changes if we kept just one
progress bar with text inside it. We can also remove a few CSS rules
which we added to override foundation's rules for the
`progress-meter-text` class.
We were setting it to 0, and so screen reader users might be confused by
it.
The easiest way to reuse the code and using it for both this attribute
and the width of the progress bar is to move this method to the voting
style, just like the other methods used in this view.
Note the progressbar ARIA role might not be right, since this isn't a
task which is "progressing", but an indicator of the amount spent and
amount available, which is exactly what the <meter> HTML5 tag was
designed for.
We might use a <meter> tag in the future. For now, I'm leaving it as it
is because I'm not certain about how well <meter> is supported in
accessibility tools, and because it's definitely not supported in
Internet Explorer 11, which we haven't officially dropped support for.
We were rendering an individual ballot, and then rendering all ballots
(including the already rendered one). So we can skip the first part, as
pointed out by microweb10 in the comments of pull request 3036.
In the Knapsack voting style, we can't add an investment if its cost is
greater than the money we've got left, but in other voting styles money
might not be the issue.
So we're introducing the term "resources" and adapting the code
accordingly.
We're passing the amount as a paramenter to the "remaining" text, so it
makes sense to pass it to the "amount spent" text as well.
Here we're also changing the I18n key to the text saying users can
change their vote, so it's easier to note the text is about changing
their vote, and not about the projects they have voted so far.
Since we're going to introduce a new voting style which will not be
based on money, we're extracting the logic specific to the current
voting style to a new class.
This way adding new voting styles will be easier.
The heading is used with `find_by_slug_or_id`, which raises an exception
if it isn't found, so executing `@heading.group` after it does not need
the safe navigation operator.
We were using the same logic twice.
I've moved the logic to the Ballot model, which to me is a more natural
place to calculate whether there's enough money left than the Investment
model. After all, the remaining money is in the ballot, and not in the
investment.
The `refresh_ballots` partial ignores the `investment` parameter
completely; instead, it iterates over the investments in the
`@investments` instance variable.
One method was calling `reason_for_not_being_ballotable_by` passing just
one parameter instead of two.
The other method was calling the method `amount_spent`, which does not
exist in the Budget class.
So both methods would make the application crash if they were called.
Luckily, they aren't, so the application doesn't crash.
