Partial replacement #468

kristianpd · 2015-02-27T18:51:20Z

This closes #448 by adding partial replace capabilities to Turbolinks.

… right now

Thibaut · 2015-02-27T18:51:37Z

tldr;

<body>
  <div id="sidebar" data-turbolinks-permanent>
    Sidebar, never changed!
  </div>

  <div id="flash" data-turbolinks-temporary>
    You have new comments. <%= link_to 'See new comments', comments_path, change: 'comments' %>
  </div>

  <section id="comments:collection">
    <article id="comment_123"></article>
  </section>

  <div>
    <a id="comments:link">See 45 more comments</a>
  </div>

  <%= form_for Comment.new, remote: true, id: 'new_comment' do |form| %>
    <% # Show validation errors %>

    <%= form.text_area :content %>
    <%= form.submit %>
  <% end %>
</body>

<script>
# Will change #flash, #comments:collection, #comments:link
Turbolinks.visit(url, change: 'comments')

# Will change #flash, #comment_123
Turbolinks.visit(url, change: 'comment_123')

# Will only keep #sidebar
Turbolinks.visit(url)

# Will only keep #sidebar, #flash
Turbolinks.visit(url, keep: 'flash')

# Will keep nothing
Turbolinks.visit(url, flush: true)

# Same as visit() but takes a string
Turbolinks.replace(html, options)
</script>

class CommentsController < ActionController::Base
  def index
    # renders, but keeps #sidebar
  end

  def create
    @comment = Comment.new(comment_params)

    if @comment.save
      # This will change #flash, #comments:collection, #comments:link
      # -> Turbolinks.visit('/comments', change: 'comments')
      redirect_to comments_url, change: 'comments'
    else
      # Validation failure
      # -> Turbolinks.replace('<%=j render :new %>', change: 'new_comment')
      render :new, change: :new_comment
    end
  end
end

dhh · 2015-02-27T18:52:49Z

lib/turbolinks/redirection.rb


+    private
+


✂️ newline, indent all below private.

dhh · 2015-02-27T18:55:52Z

Lovely. For the record, this goes into master under the target that master is now targeting a 3.0 release.

Partial replacement

arthurnn · 2015-02-27T19:01:25Z

🎉 ❤️ 💚 💛 😍

aantix · 2015-02-27T19:05:33Z

Fantastic @kristianpd

kmcphillips · 2015-02-27T19:12:56Z

🚀 🌙

Thibaut · 2015-02-28T18:08:22Z

FYI me and @kristianpd will write docs for this next week.

Thibaut · 2015-02-28T18:12:18Z

And I'm also going to try to add some automated JS tests.

kurtfunai · 2015-03-02T16:52:32Z

👍 🍰

reed · 2015-03-03T19:17:54Z

As it is now, Turbolinks.visit(url, change: 'comments') won't change the current URL. I see the value of that, but I also see scenarios where I'd want the URL to change. Would anyone object if I added the ability to specify that you want it to change?

Something like this:

Turbolinks.visit(url, change: 'search-results', change_url: true)

@Thibaut @kristianpd @dhh

dhh · 2015-03-03T19:46:34Z

Why wouldn’t Turobolinks.visit not always change the url, even when using change? Change just says “wherever we’re going, we’re just only going to change this specific DOM element”.

On Mar 3, 2015, at 11:17 AM, Nick Reed notifications@github.com wrote:

As it is now, Turbolinks.visit(url, change: 'comments') won't change the current URL. I see the value of that, but I also see scenarios where I'd want the URL to change. Would anyone object if I added the ability to specify that you want it to change?

Something like this:

Turbolinks.visit(url, change: 'search-results', change_url: true)
@Thibaut https://github.com/Thibaut @kristianpd https://github.com/kristianpd @dhh https://github.com/dhh
—
Reply to this email directly or view it on GitHub #468 (comment).

reed · 2015-03-03T21:13:47Z

Yeah, I thought I saw the value of not changing the URL, but the more I think about it, I can't come up with a scenario where you wouldn't want it to change. Not that that means there isn't one.

The relevant code currently looks like this:

if doc = processResponse()
  reflectNewUrl url unless options.change? || options.keep? || options.flush?
  reflectRedirectedUrl() # will change url if server redirected
  # ...
else
  document.location.href = crossOriginRedirect() or url.absolute # will change url

If change/keep/flush are set, issues arise in two cases. First, if processResponse() returns false, then the URL is going to change, but if it returns a valid document, then it won't...unless the second case happens, where the server has redirected. Then the URL will be changed by reflectRedirectedUrl(), which will actually use history.replaceState and overwrite the current state (which was supposed to be the state created by reflectNewUrl had it not been skipped).

Thibaut · 2015-03-04T13:05:11Z

I agree Turbolinks.visit should always change the url, and Turbolinks.replace should never.
Also agree with @reed that a redirect_to url, change: :foo should push a state, not replace the current one.

@kristianpd what was your reasoning behind reflectNewUrl url unless options.change?
https://github.com/rails/turbolinks/blob/34a426741f07eaac9fa59718c9e8219933f42dfe/lib/assets/javascripts/turbolinks.js.coffee#L69

It would be good to add some tests for these scenarios. I can work on that maybe tonight or this week-end.

kristianpd · 2015-03-04T14:32:10Z

@Thibaut TBH It's from Turbograft but now that I think about it, the differentiator should really be whether you visit or replace. Since turbograft doesn't have the change option on .visit, it sort of implies that the only time you're doing a partial replace is when you're not changing the URL. Since Turbolinks may want to change the URL and keep things like the sidebar i propose:

Turbolinks.visit - Changes URL always (it may stay the same, but we remove that block)
Turbolinks.replace - Does not call reflectNewUrl

Thibaut · 2015-03-04T14:34:43Z

@kristianpd that makes sense 👍

reed · 2015-03-04T20:32:24Z

Made the change: 7387a96

kristianpd and others added 30 commits February 27, 2015 08:43

V1 partial replace

21adb6a

Switch to ID prefix change keys

e3a60e8

Don't replaceState on partial updates

d81ce5f

Adds data-turbolinks-temporary attribute and refresh behaviour

b4ad476

Adds Turbolinks.replace and fires the right events

df850bc

Adds remaining API elements to Turbolinks

86f1b96

Add flush: true to Turbolinks.visit

29e55ac

Make redirect_to perform a turbolinks visit on xhr+non-get requests

04f9a5c

Add :change option to redirect_via_turbolinks_to

6bf6692

Add :change option to redirect_to

0de6403

redirect_via_turbolinks_to -> redirect_to turbolinks: true

200c43c

Add :keep option to redirect_to

3ae0b0e

Refactor tests

10f2dd0

Add :flush option to redirect_to

f38553a

Use HTML string instead of document as option to replace

e441b85

Implement redirect_to turbolinks: false

499c4d1

Remove unneecessary extend ActiveSupport::Concern

c97e864

Be explicit about which body's are being used

d74563a

Always use document with changePage

7c9d6b2

Use new changePage method signature

2a78a20

Cleanup node finding

ccf16e2

Cleanup the keep nodes path

23f570f

Add render turbolinks: true/false

593f522

Add render :change/:keep

e2149af

Add render :flush

54be457

Don't change the return value of render

089a046

Bring back redirect_via_turbolinks_to with a deprecation warning

e9fb49d

Flatten keep node logic

31047f8

Simplify changeNodes a bit, be clear about targetBody

b05b389

Use better variable names, get rid of redundant body assignment

31cc2b5

Thibaut and others added 5 commits February 27, 2015 13:38

Refactor Turbolinks::Redirection

e094770

Remove accidental changes and Turbograft stuff that may not be needed…

ef18670

… right now

Merge keepNodes and changeNodes into one path

69552b6

Minor changes to turbolinks.js.coffee

087ce7a

Find nodes by prefix or exact match

3cf2bf9

dhh reviewed Feb 27, 2015
View reviewed changes

lib/turbolinks/redirection.rb

private

Copy link

Contributor

dhh Feb 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️ newline, indent all below private.

dhh added a commit that referenced this pull request Feb 27, 2015

Merge pull request #468 from Shopify/turboer-links

ad6ef8b

Partial replacement

dhh merged commit ad6ef8b into turbolinks:master Feb 27, 2015

arthurnn deleted the turboer-links branch February 27, 2015 18:56

Thibaut mentioned this pull request Feb 28, 2015

Add Ruby tests #470

Merged

Thibaut mentioned this pull request Mar 1, 2015

Add JS tests #472

Merged

carlalexander mentioned this pull request Mar 6, 2015

Add support for partial page replacement strategies with Turbograft helthe/Turbolinks#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial replacement #468

Partial replacement #468

kristianpd commented Feb 27, 2015

Thibaut commented Feb 27, 2015

dhh Feb 27, 2015

dhh commented Feb 27, 2015

arthurnn commented Feb 27, 2015

aantix commented Feb 27, 2015

kmcphillips commented Feb 27, 2015

Thibaut commented Feb 28, 2015

Thibaut commented Feb 28, 2015

kurtfunai commented Mar 2, 2015

reed commented Mar 3, 2015

dhh commented Mar 3, 2015

reed commented Mar 3, 2015

Thibaut commented Mar 4, 2015

kristianpd commented Mar 4, 2015

Thibaut commented Mar 4, 2015

reed commented Mar 4, 2015

Partial replacement #468

Partial replacement #468

Conversation

kristianpd commented Feb 27, 2015

Thibaut commented Feb 27, 2015

dhh Feb 27, 2015

Choose a reason for hiding this comment

dhh commented Feb 27, 2015

arthurnn commented Feb 27, 2015

aantix commented Feb 27, 2015

kmcphillips commented Feb 27, 2015

Thibaut commented Feb 28, 2015

Thibaut commented Feb 28, 2015

kurtfunai commented Mar 2, 2015

reed commented Mar 3, 2015

dhh commented Mar 3, 2015

reed commented Mar 3, 2015

Thibaut commented Mar 4, 2015

kristianpd commented Mar 4, 2015

Thibaut commented Mar 4, 2015

reed commented Mar 4, 2015