Add flush option for Sinatra::ContentFor flush content #1225

iguchi1124 · 2016-12-30T06:31:46Z

I added flush option to content_for method for flush content. which like content_for :flush options of Rails.

see: http://api.rubyonrails.org/classes/ActionView/Helpers/CaptureHelper.html#method-i-content_for

content_for :something_else do
  :that_one_thing
end

# flush option
content_for :something_else, flush: true do
  yield_content :that_other_thing
end

yield_content :something_else # returns content :that_other_thing

jkowens · 2017-01-17T16:05:49Z

@iguchi1124 I don't think that the way you've implemented :flush quite lines up with the Rails behavior.

This is how I would expect flush to behave:

# flush option
content_for :something_else do
  :that_one_thing
end

content_for :something_else, flush: true do
  :that_other_thing
end

yield_content :something_else # returns content :that_other_thing
yield_content :something_else # returns content :that_other_thing

So instead of just rendering the content one time, it renders only the last content with flush: true every time afterwards.

jkowens · 2017-01-17T16:56:20Z

sinatra-contrib/lib/sinatra/content_for.rb

@@ -128,9 +129,25 @@ module ContentFor
    #
    # Your blocks can also receive values, which are passed to them
    # by <tt>yield_content</tt>
-    def content_for(key, value = nil, &block)
+    def content_for(key, *args, &block)


I would rather see the method signature:

def content_for(key, value = nil, options = {}, &block)

It would avoid the need to jump through the hoops below.

jkowens · 2017-01-17T22:36:42Z

sinatra-contrib/lib/sinatra/content_for.rb

    end

    private

-    def content_blocks
-      @content_blocks ||= Hash.new { |h, k| h[k] = [] }
+    def contents


I kind of like the method name content_blocks, I feel like it is a bit more descriptive of what the hash represents.

jkowens · 2017-01-17T22:50:49Z

sinatra-contrib/lib/sinatra/content_for.rb

      block ||= proc { |*| value }
-      content_blocks[key.to_sym] << capture_later(&block)
+      flush_key_set << key if options[:flush]


I think this could be simplified to just something like:

clear_content_for(key) if options[:flush]

I'm almost certain that would take care of mimicking the Rails behavior.

jkowens · 2017-01-19T05:49:00Z

@iguchi1124 thanks for getting started on this feature! I think you're close, just a few tweaks should do it 👍

iguchi1124 · 2017-01-19T06:52:05Z

I'll try to fix the review points soon, thanks for reviews!

iguchi1124 · 2017-02-06T17:18:54Z

Thank you for teaching me, I was misunderstanding about that options meanings.
I fixed this pull request.

@jkowens @zzak

zzak · 2017-03-27T23:51:18Z

Thank you for updating your patch @iguchi1124!

I will postpone this until after 2.0.0 release is made.

javimbk · 2017-08-17T15:00:00Z

Hello! Is this finally making it into Sinatra? @zzak

iguchi1124 · 2018-02-19T17:35:19Z

Hi there, do you have any progress regarding this releases or reviews?

jkowens · 2018-12-21T17:00:31Z

@namusyaka if this can be rebased, can we schedule this for the 2.1 release?

jkowens · 2018-12-21T17:05:57Z

sinatra-contrib/lib/sinatra/content_for.rb

      block ||= proc { |*| value }
-      content_blocks[key.to_sym] << capture_later(&block)
+      clear_content_for(key) if options[:flush]


@iguchi1124 this is the only line that needs added in this method. Can you rebase with master and limit changes to just this line and the added tests?

Just tried to resolve the conflicts by web GUI.

namusyaka · 2018-12-21T17:14:47Z

I don't have to wait until 2.1 release because the public interface seems not to be broken by this patch. The internal key symbolization isn't problem, I think.

jkowens · 2018-12-21T17:19:39Z

sinatra-contrib/lib/sinatra/content_for.rb

      if block_given? && !content_for?(key)
        haml? ? capture_haml(*args, &block) : yield(*args)
      else
-        content = content_blocks[key.to_sym].map { |b| capture(*args, &b) }
+        content = content_blocks[key].map { |b| capture(*args, &b) }


This change doesn't seem necessary to me. What do you think?

jkowens · 2018-12-21T17:21:01Z

sinatra-contrib/lib/sinatra/content_for.rb

      block ||= proc { |*| value }
-      content_blocks[key.to_sym] << capture_later(&block)
+      clear_content_for(key) if options[:flush]
+      content_blocks[key] << capture_later(&block)


I would rather leave this as:

content_blocks[key.to_sym] << capture_later(&block)

and delete the assignment key = key.to_sym above.

namusyaka · 2018-12-21T17:26:32Z

@jkowens Your reviews should be reflected after merging this.

### sinatra-contrib * Add `flush` option to `content_for` [#1225](sinatra/sinatra#1225) by Shota Iguchi * Drop activesupport dependency from sinatra-contrib [#1448](sinatra/sinatra#1448) * Update `yield_content` to append default to ERB template buffer [#1500](sinatra/sinatra#1500) by Jordan Owens

iguchi1124 changed the title ~~[WIP] Add flush option for Sinatra::ContentFor flush content~~ Add flush option for Sinatra::ContentFor flush content Dec 30, 2016

iguchi1124 mentioned this pull request Dec 30, 2016

Fixed twitter-text version to 1.14.0 #1226

Merged

jkowens reviewed Jan 17, 2017

View reviewed changes

zzak added the feature label Jan 30, 2017

zzak added this to the 2.0.0 milestone Jan 30, 2017

Add flush option to content_for

cb45e46

zzak modified the milestones: Beyond, 2.0.0 Mar 27, 2017

jkowens reviewed Dec 21, 2018

View reviewed changes

Merge branch 'master' into flush-content-for

d75c242

jkowens reviewed Dec 21, 2018

View reviewed changes

namusyaka merged commit 84b290a into sinatra:master Dec 21, 2018

namusyaka added a commit that referenced this pull request Dec 21, 2018

follow up #1225

69956da

iguchi1124 deleted the flush-content-for branch December 24, 2018 06:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flush option for Sinatra::ContentFor flush content #1225

Add flush option for Sinatra::ContentFor flush content #1225

iguchi1124 commented Dec 30, 2016 •

edited

Loading

jkowens commented Jan 17, 2017 •

edited

Loading

jkowens Jan 17, 2017

jkowens Jan 17, 2017

jkowens Jan 17, 2017

jkowens commented Jan 19, 2017

iguchi1124 commented Jan 19, 2017

iguchi1124 commented Feb 6, 2017 •

edited

Loading

zzak commented Mar 27, 2017

javimbk commented Aug 17, 2017

iguchi1124 commented Feb 19, 2018

jkowens commented Dec 21, 2018

jkowens Dec 21, 2018

namusyaka Dec 21, 2018

namusyaka commented Dec 21, 2018

jkowens Dec 21, 2018

jkowens Dec 21, 2018

namusyaka commented Dec 21, 2018

Add flush option for Sinatra::ContentFor flush content #1225

Add flush option for Sinatra::ContentFor flush content #1225

Conversation

iguchi1124 commented Dec 30, 2016 • edited Loading

jkowens commented Jan 17, 2017 • edited Loading

jkowens Jan 17, 2017

Choose a reason for hiding this comment

jkowens Jan 17, 2017

Choose a reason for hiding this comment

jkowens Jan 17, 2017

Choose a reason for hiding this comment

jkowens commented Jan 19, 2017

iguchi1124 commented Jan 19, 2017

iguchi1124 commented Feb 6, 2017 • edited Loading

zzak commented Mar 27, 2017

javimbk commented Aug 17, 2017

iguchi1124 commented Feb 19, 2018

jkowens commented Dec 21, 2018

jkowens Dec 21, 2018

Choose a reason for hiding this comment

namusyaka Dec 21, 2018

Choose a reason for hiding this comment

namusyaka commented Dec 21, 2018

jkowens Dec 21, 2018

Choose a reason for hiding this comment

jkowens Dec 21, 2018

Choose a reason for hiding this comment

namusyaka commented Dec 21, 2018

iguchi1124 commented Dec 30, 2016 •

edited

Loading

jkowens commented Jan 17, 2017 •

edited

Loading

iguchi1124 commented Feb 6, 2017 •

edited

Loading