Always trigger page:before-change on visit or replace + passing Scroll param from server to Javascript

[vizo]

Always trigger page:before-change and provide also nodesToChange in event.data.

Passing scroll param from server to Javascript.

[tortuetorche]

Hi @vizo,

Thank you for your contributions, can you also update the tests of Turbolinks?
Because actually, they failed, see: https://travis-ci.org/rails/turbolinks/builds/75558928

Have a good day,
Tortue Torche

[vizo]

Hi @tortuetorche, you are welcome. Now is passing.

[Thibaut]

Could you open separate PRs for each change and add tests please.

[vizo]

I can make separate PRs, but i don't know how, sorry. But it's time to learn :) Google said that i must do new branch with each change and then request PR on each branch? Is that correct? About tests, i am also new ... but i think there should be just few new tests, so i can try.

[Thibaut]

• implemented the page:before-change change in 6d4f600
• see my comment on redirect_to and scroll to acnhor #589 (comment)
• feel free to open a new PR for the last commit (with a test case please)

[vizo]

There are two more things:

• In array of nodesToChange, elements with data-turbolinks-temporary attribute are dupliacted. This causes error and javascript stops (my PR solved that).
• page:before-change event.data doesn't hold list of affected elements, just url.
  So for example, if form on page is changed by user, you can't detect if form is child of elements that will be replaced in DOM, so you can't show confirmation box)

[Thibaut]

In array of nodesToChange, elements with data-turbolinks-temporary attribute are dupliacted. This causes error and javascript stops (my PR solved that).

Please open a new PR with a test case.

page:before-change event.data doesn't hold list of affected elements, just url.

This is intended. page:before-change is only for preventing visits (not partial replacements), in which case there are no affected elements (yet). Changing that would break backward compatibility.

So for example, if form on page is changed by user, you can't detect if form is child of elements that will be replaced in DOM, so you can't show confirmation box

You can use the page:change event to detect that.

[vizo]

Ok clear. But as i know page:change event is not cancellable with event.preventDefault(), like event page:before-change? So how can i know first which elements will be changed and then prevent it?

[Thibaut]

Why are you trying to prevent it?

[vizo]

I want to prevent it if form is dirty and user clicks cancel in confirmation box ("Your changes will be lost. Continue?").

Use case:

1. Page with form
2. User do some input but doesn't submit yet (makes dirty form).
3. User clicks on some other link (turbolink link, so some parts of page will be changed).
4. page:before-change is triggered
5. Inside event function you can show confirmation box just if elements that will be replaced in DOM affects dirty form, if not, don't show confirmation at all and change DOM.

[Thibaut]

5 Inside event function you can show confirmation box just if elements that will be replaced in DOM affects dirty form, if not, don't show confirmation at all and change DOM.

In my opinion this is too much of an edge case to justify the additional complexity in Turbolinks (if we add a new event). You can probably achieve the same behavior by checking which link was clicked and/or the URL (or just showing the confirmation box all the time).

[vizo]

I accept you opinion but it's sad because this data is already available, just not passed to an event.
What i wrote is just one case. It's very useful when you have more forms on same page. I am not asking for new event, but for more information in event.data.

Believe me i tried a lot of workarounds before I made PR. I agree that you can do it with click events on all other links, but not all links are navigating away to other page in real application (some have just js events, also sometimes they are not links, just elements with click events, also canceled events and so on...). It becomes messy. Another problem is binding as a first event to trigger, before others (ugly workarounds to insert it as first, source: google). Also it doesn't cover if user press back button.

page:before-change covers it all and it's right place to cancel or allow page changes (like it is), But my opinion is, we should really know what changes will be made in DOM, so we can decide smart (to allow or not). I guess that's why event.preventDefault() is available? But how can we decide smart just with URL (i also use multilingual URLs for SEO in my app)? From my point of view, URL information on client side is useless in real application (unless you use client side router). Correct me if i am wrong.

Imagine keypress event without telling you which key was pressed. I feel the same :)

Also i don't see a reason why not all page:* events have event.data object of all parameters (like "change", "scroll", "url", ..) to make it flexible for all use cases. This means unique API for all events and also more backward compatibility in the future, because more params could be added.

Turbolinks could be very good substitute of other client side frameworks.

[Thibaut]

At the time that page:before-change is fired, we don't know yet which elements are going to get swapped (the server could return a Turbolinks.visit/replace response), unless you manually called Turbolinks.visit(url, change: ['key']). It would be confusing to pretend otherwise.

If you called Turbolinks.visit(url, change: ['key']) manually, then I don't see what the problem is. You can just as easily not call visit based on the change key that that code intends to pass.

If you really care about this, it's easy to patch Turbolinks to do what you want:

originalVisit = Turbolinks.visit
Turbolinks.visit = (url, options) ->
  return if # fire new event with options.change and check if prevented
  originalVisit()

[vizo]

In my PR i moved page:before-change to place where you already know which elements are going to be changed (it is some logic behind, temp, keep, ...).

I don't call it manually.

I made patch before, but there you don't know all elements that will be changed (just listed). I must write logic for temp and keep and so on, so this is far from DRY. I can't use turbolinks logic out of the box.

Looks like i will need to use my fork then.
Thanks anyway for your time.