Skip to content
This repository

Extend date_select helper functionality. #5553

merged 0 commits into from over 1 year ago

7 participants

Pavel Nikitin Andrey Koleshko Steve Klabnik Niels R. Damien Mathieu Rafael Mendonça França Carlos Antonio da Silva
Pavel Nikitin

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

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

Hope it will by helpfully.

Andrey Koleshko

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.

Steve Klabnik

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

Niels R.

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


Damien Mathieu

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

Rafael Mendonça França

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 :+1: to add theses classes by default, but some people will want to not generate they.

... ...
@@ -922,6 +924,7 @@ def build_select(type, select_options_as_html)
922 924
             :name => input_name_from_type(type)
923 925
924 926
           select_options.merge!(:disabled => 'disabled') if @options[:disabled]
+          select_options.merge!(:class => type) if @options[:css_by_type]

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Rafael Mendonça França

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

Rafael Mendonça França rafaelfranca closed this October 14, 2012
Pavel Nikitin

@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.

Rafael Mendonça França rafaelfranca reopened this October 15, 2012
Pavel Nikitin butcher merged commit c432c74 into from October 17, 2012
Pavel Nikitin butcher closed this October 17, 2012
Pavel Nikitin

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

Sorry, commit information is not available for this pull request.

This page is out of date. Refresh to see the latest.

Showing 0 changed files with 0 additions and 0 deletions. Show diff stats Hide diff stats


Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.