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

Added label_wrapper_tags #73

Closed
wants to merge 4 commits into from
Closed

Conversation

sirlori
Copy link

@sirlori sirlori commented Oct 26, 2014

I think that the readme is self-explanatory

@rafaelfranca
Copy link
Collaborator

I may be wrong but this is not doing exactly what :label_tag option does?

@sirlori
Copy link
Author

sirlori commented Oct 26, 2014

@rafaelfranca
:label_tag I think that does it in the initializer I think, and just with one wrapper though.

@rafaelfranca
Copy link
Collaborator

:label_tag is also present in the option for the label method. About being just one wrapper I believe it is fine, if you need more wrappers you should use normal Rails helpers.

@rafaelfranca
Copy link
Collaborator

I mean, could you explain why using:

a.label :name, l_wrapper_tags: :h2

instead of:

content_tag :h2 do
  a.label :name
end

Not saying that your feature is not useful, I'm just trying to understand how it is useful.

Thank you for the pull request ❤️

@sirlori
Copy link
Author

sirlori commented Oct 26, 2014

That's a point. I've been developing in rails for 2 weeks and I don't know all the stuff of course.
I find what you are saying true and I could have done differently, but I didn't want to leave show_for. Because of the internalization and also i think that it is simpler to write like I've done.
If you think that multiple wrappers for a label aren't useful just go to the boostrap page and find out that the only way to increase the size of a label is to put it in an h tag.
Sorry for the bad english, ITALIAN HERE

@sirlori
Copy link
Author

sirlori commented Oct 26, 2014

Also I've modified the version... If it is a problem don't merge it, if you will ever merge it :DD

@rafaelfranca
Copy link
Collaborator

I believe the best thing to do is to use normal Rails helpers. It is easier to understand without even having to search what means :label_wrapper_tags options. You are just explicitly writing the wrapper code. Also you are not leaving show_for, you are still using it since you are calling the label method of show_for, so you still have the internationalisation and everything show_for provides to you.

@carlosantoniodasilva @josevalim WDYT?

@sirlori
Copy link
Author

sirlori commented Oct 26, 2014

That works fine if you don't have to write something like this:

content_tag :h2 do
  a.label :name
  a.attribute :name, label: nil # or false if you want
end

because this would wrap also the attribute into a h2

if you try to resolve that by writing this:

content_tag :h2 do
  a.label :name
end
a.attribute :name, label: nil # or false if you want

this won't write the label into the whole content wrapper.
I can't fix that. I agree that it is a limit case but I personally need that, don't know if others do as well.

@carlosantoniodasilva
Copy link
Member

I'm happy leaving the helpers generation for Rails, Show For already handles a single wrapper which is a good default, everything else can be done by helpers/Rails.

Thanks for your pull request. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants