Skip to content

Conversation

@butcher
Copy link
Contributor

@butcher butcher commented Mar 22, 2012

Often we need to have date selectors with different styles. (Most often different width.) Unfortunately date_select helper don't provide possibility to set different CSS classes. This commit extend date_select helper with option :css_by_type that set special classes for generated 'select' tags. Class names equal data type, for example:
f.date_select :birthday, :css_by_type => true
generate:

<select class='month ...>...</select>
<select class='day' ...>...</select>
<select class='year' ...>...</select>

Hope it will by helpfully.

@ka8725
Copy link
Contributor

ka8725 commented Mar 27, 2012

You have a typo in description of the method - assing instead of assign. Also I think param name css_by_type is not suitable here. generate_classes or something like this would be better. It would be nice feature if somebody merged it.
+1

@steveklabnik
Copy link
Member

This will no longer merge cleanly, and will need a rebase.

@nlsrchtr
Copy link

@butcher: Would be great to see this kind of feature in the date_select and date_time_select helpers.

👍

@dmathieu
Copy link
Contributor

I like this. However, having it all the time wouldn't break anything.
Why not make it the default (without any option for it).

@rafaelfranca
Copy link
Member

Good question. This is a controversial change. Working in SimpleForm some guys complaint about all the automatically generated classes, some guys like and use those classes.

I'm 👍 to add theses classes by default, but some people will want to not generate they.

Choose a reason for hiding this comment

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

Perhaps the option could be add_classes or css_classes? Not sure what's the best name - or if there are other better possibilities.

Lets try to move this forward so we can get it merged, thanks.

@rafaelfranca
Copy link
Member

Two months without answers. I'm closing this one. Feel free to ask to reopen if you want to work on it.

@butcher
Copy link
Contributor Author

butcher commented Oct 15, 2012

@rafaelfranca perhaps I miss notification about your previous comment. I think we should make decision together how to implement this with the best interface. So as I see we have some solutions:

  1. Make this functionality default
  2. Create special option
    • add_classes
    • css_classes
    • with_css_classes

I vote for creating option with_css_classes

Please share your opinion and reopen the pull request. Than I reimplement it and prepare valid pull request.

I hope it will be helpful for developers that use rails.

@rafaelfranca rafaelfranca reopened this Oct 15, 2012
@butcher butcher merged commit c432c74 into rails:master Oct 17, 2012
@butcher
Copy link
Contributor Author

butcher commented Oct 17, 2012

Something strange happens when I try to rebase from rails/rails.

Funny github:

  • wrote that I "merge commit to rails:master" (but I have no access to this repo)
  • close my pull request

So I open new one. You can review my changes here #7975

Sorry for cutting discussion.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants