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

Split RecordTagHelper into a gem and remove from Rails #18337

Closed
dhh opened this issue Jan 5, 2015 · 21 comments
Closed

Split RecordTagHelper into a gem and remove from Rails #18337

dhh opened this issue Jan 5, 2015 · 21 comments
Milestone

Comments

@dhh
Copy link
Member

dhh commented Jan 5, 2015

I remember adding RecordTagHelper as an experiment, but never felt happy with its use in real life. Let's kick it out in a gem and remove it from core.

@dhh dhh added the actionpack label Jan 5, 2015
@dhh dhh added this to the 5.0.0 milestone Jan 5, 2015
@todd
Copy link
Member

todd commented Jan 6, 2015

I can take a look at this if no one else was planning on taking it.

@dhh
Copy link
Member Author

dhh commented Jan 6, 2015

Please do, Todd!

On Jan 5, 2015, at 8:34 PM, Todd Bealmear notifications@github.com wrote:

I can take a look at this if no one else was planning on taking it.


Reply to this email directly or view it on GitHub #18337 (comment).

@todd
Copy link
Member

todd commented Jan 6, 2015

@dhh Taking a quick look, it looks like the only place ActionView::RecordIdentifier is being used is in RecordTagHelper - do you also want me to extract that code or leave it be for now?

@dhh
Copy link
Member Author

dhh commented Jan 6, 2015

I’ve used #dom_id a bunch. That seems like the right level of abstraction. div_for and content_for went too high. So let’s keep that in.

On Jan 5, 2015, at 8:54 PM, Todd Bealmear notifications@github.com wrote:

@dhh https://github.com/dhh Taking a quick look, it looks like the only place ActionView::RecordIdentifier is being used is in RecordTagHelper - do you also want me to extract that code or leave it be for now?


Reply to this email directly or view it on GitHub #18337 (comment).

@todd
Copy link
Member

todd commented Jan 6, 2015

👍

@carlosantoniodasilva
Copy link
Member

David, I've seen content_tag_for widely used by many apps, and it has a simple implementation that has close to 0 cost of maintenance. I think there is no need to remove it from core. We could, however, deprecate div_for in my opinion, as it does not add much value on top of the former.

@dhh
Copy link
Member Author

dhh commented Jan 6, 2015

I'm not a fan of it. It seems anemic to me over just doing something like content_tag :div, id: dom_id(record) do/end.

@carlosantoniodasilva
Copy link
Member

Well, technically it does add both id and class using dom_id and dom_class respectively. In any case, it is also useful for collections:

content_tag_for :div, @posts do/end

# which would be something like:

@posts.each do |post|
  content_tag :div, id: dom_id(post), class: dom_class(post) do/end
end

If you still feel skeptical about having it in core then I think I have no more arguments on this, other than extracting to a gem in case people want it back while upgrading :).

@dhh
Copy link
Member Author

dhh commented Jan 6, 2015

I don’t like the look of that as it is. I remain 👍 on going gem on it.

On Jan 6, 2015, at 10:37, Carlos Antonio da Silva notifications@github.com wrote:

Well, technically it does add both id and class using dom_id and dom_class respectively. In any case, it is also useful for collections:

content_tag_for :div, @posts do/end

which would be something like:

@posts.each do |post
|
content_tag
:div, id: dom_id(post), class: dom_class(post) do/end
end
If you still feel skeptical about having it in core then I think I have no more arguments on this, other than extracting to a gem in case people want it back while upgrading :).


Reply to this email directly or view it on GitHub.

@carlosantoniodasilva
Copy link
Member

David, do you think it should initially live under the rails org or the author could manage the extracted gem under its user (and we would document it on release notes and so on, of course)?

@dhh
Copy link
Member Author

dhh commented Jan 11, 2015

I think traditionally, we’ve had the gem live under rails. That seems fine to continue.

On Jan 11, 2015, at 1:34 PM, Carlos Antonio da Silva notifications@github.com wrote:

David, do you think it should initially live under the rails org or the author could manage the extracted gem under its user (and we would document it on release notes and so on, of course)?


Reply to this email directly or view it on GitHub #18337 (comment).

@carlosantoniodasilva
Copy link
Member

👍

@rafaelfranca
Copy link
Member

👍 for rails organization. If author want to manage the release and the repository it is fine, we can give him the access, as long we can do also. BTW, the copyright should still be the same as Rails since it an extraction of code (of course with proper attribution to the current author).

@todd
Copy link
Member

todd commented Jan 12, 2015

I'll commit to maintaining the gem moving forward - just let me know how you guys would prefer to handle that. As far as copyrights and things go, I can take care of the LICENSE file as that was just auto-generated by GitHub, but I'd also like some feedback on author attribution in the gemspec - I just put all my own information in there for now as placeholders. Alternatively, if someone would like to take care of those updates themselves, I'd be more than happy to add them as a contributor on the temporary repo.

@todd
Copy link
Member

todd commented Jan 17, 2015

@rafaelfranca Any more thoughts on author attribution?

@todd
Copy link
Member

todd commented Jan 29, 2015

Hey all - just wanted to loop back on this. Let me know how I should proceed.

@rafaelfranca
Copy link
Member

@todd of course the author attribution is fine. The capyright too, you just need to keep the original author and copyright attribution.

@todd
Copy link
Member

todd commented Jan 30, 2015

@rafaelfranca Original author being DHH or someone else? To clarify, I only need to change that in LICENSE?

@rafaelfranca
Copy link
Member

Original author is David. And gemspec and licence needs to be changed

On Thu, Jan 29, 2015, 22:11 Todd Bealmear notifications@github.com wrote:

@rafaelfranca https://github.com/rafaelfranca Original author being DHH
or someone else? To clarify, I only need to change that in LICENSE?


Reply to this email directly or view it on GitHub
#18337 (comment).

@todd
Copy link
Member

todd commented Jan 30, 2015

@rafaelfranca Ok, I took care of both of those files.

@rafaelfranca
Copy link
Member

Closed by #18411

edwardloveall added a commit to edwardloveall/portfolio that referenced this issue Nov 14, 2016
`content_tag_for` has been deprecated in rails 5:
rails/rails#18337

So this:

```erb
<%= content_tag_for(:li, @projects) do |project| %>
  <!-- display a project -->
<% end %>
```

should now be this:

```erb
<% @projects.each do |project| %>
  <%= content_tag(:li, id: dom_id(project)) do %>
    <!-- display a project -->
  <% end %>
<% end %>
```

`dom_id` will use the same ID that content_tag_for used to.
tmiller pushed a commit to tmiller/network that referenced this issue Apr 12, 2017
Rails 5 removed the view helper `#content_tag_for` and moved it into the
record_tag_helper gem. This adds support back to Rails 5 for
`#content_tag_for`.

rails/rails#18337
tmiller pushed a commit to tmiller/network that referenced this issue Apr 15, 2017
Rails 5 removed the view helper `#content_tag_for` and moved it into the
record_tag_helper gem. This adds support back to Rails 5 for
`#content_tag_for`.

rails/rails#18337
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants