Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add any/all support for variants #13470

Merged
merged 1 commit into from
Dec 31, 2013
Merged

Conversation

lukaszx0
Copy link
Member

Like format.any, you can do the same with variants now.

It works for both inline:

respond_to do |format|
  format.html.any   { render text: "any"   }
  format.html.phone { render text: "phone" }
end

and block syntax:

respond_to do |format|
  format.html do |variant|
    variant.any(:tablet, :phablet){ render text: "any" }
    variant.phone { render text: "phone" }
  end
end

Regarding implementation, VariantFilter was removed and VariantCollector is used for both inline and block syntax. This is needed, as now we don't know what variant should be rendered until we collect all of them (any complicates this).

In the end, I think this is even better. Implementation is cleaner and more consistent because we don't have 2 different things - filter and collector - but only one, which takes care of everything.

/cc @dhh

@ghost ghost assigned lukaszx0 Dec 24, 2013
@dhh
Copy link
Member

dhh commented Dec 24, 2013

Will format.any(:none, :phone) work as well?

@lukaszx0
Copy link
Member Author

It will but for format, not variant. Formats have its own collector (for mime type negotiation) and it implements it's own any method here: https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/metal/mime_responds.rb#L446-L453

I've just added test case which should clarify you whole the thing: lukaszx0@3c33f36#diff-a528c4ecc381b6bf63516c4f2ed063c4R233.

You could've ask also if implicit rendering will work when using any on variant but without passing a block. Yes it will and I've added test case for it as well: lukaszx0@3c33f36#diff-a528c4ecc381b6bf63516c4f2ed063c4R226

@asiniy
Copy link

asiniy commented Dec 25, 2013

It looks good, but why it's broken? @strzalek do you need a help with commit?

@lukaszx0
Copy link
Member Author

It's definitely not broken. AP and AV are passing, railties and database related tasks are failing because they're are broken after recent changes on master. We're working on fixing this, please check other PRs.

@asiniy
Copy link

asiniy commented Dec 25, 2013

okay. Where do you think I can contribute to?

@lukaszx0
Copy link
Member Author

Here's latest build on master: https://travis-ci.org/rails/rails/builds/15958111, you can check what's broken and send PR with a fix. I must say though, that because of holidays I'm not up to date and there might be PR with fixes sent already (good idea to check that fist).

Besides that, there's a ton to do! We have 700+ open issues (https://github.com/rails/rails/issues?direction=desc&labels=&sort=updated&state=open), fixing any of it would be a great help. If you've never contributed to rails this might be a bit hard and overwhelming for the first time but helping out by providing feedback for the issues, like how to reproduce it or confirming that it exists would be helpful as well. I would recommend to start with browsing issues labeled with intermediary level (https://github.com/rails/rails/issues?direction=desc&labels=Level%3AIntermediary&page=1&sort=updated&state=open), they should be fairly easy to start with. Check out also contributor's guide where you find all informations that will help you prepare your first patches (http://guides.rubyonrails.org/contributing_to_ruby_on_rails.html).

If you have any further questions do not hesitate to shoot me an email (you can find my email address on my github profile page). I'll happily answer to any!

Hope that helps, looking forward to your contributions! Thanks!

@@ -482,25 +502,31 @@ def initialize
@variants = {}
end

def any(*args, &block)
if block_given?
if args.any? && args.find_all{ |a| a == @variant }.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be simplified to:

if args.any? && args.none?{ |a| a == @variant }

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. Thanks!

@dhh
Copy link
Member

dhh commented Dec 26, 2013

Sorry, I meant variant.any(:none, :phone). Basically that you can include the none case together with named variants.

@lukaszx0
Copy link
Member Author

@dhh oh yeah, sorry. Sure, it will work. I've just added another test case to assure that (lukaszx0@a288cc1#diff-a528c4ecc381b6bf63516c4f2ed063c4R726) 😉

Like `format.any`, you can do the same with variants.

It works for both inline:

    respond_to do |format|
      format.html.any   { render text: "any"   }
      format.html.phone { render text: "phone" }
    end

and block syntax:

    respond_to do |format|
      format.html do |variant|
        variant.any(:tablet, :phablet){ render text: "any" }
        variant.phone { render text: "phone" }
      end
    end
dhh added a commit that referenced this pull request Dec 31, 2013
@dhh dhh merged commit b5fdeaa into rails:master Dec 31, 2013
@lukaszx0 lukaszx0 deleted the variants-all-any branch December 31, 2013 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants