Add `use_year_names` option to date_select tag #32190

liwii · 2018-03-07T15:27:33Z

Summary

Add use_year_names option to date_select tag.
This option makes it possible to customize year names. Example:

    date_select('user_birthday', '', start_year: 1998, end_year: 2000, use_year_names: ["Heisei 10", "Heisei 11", "Heisei 12"])

The HTML produced:

    <select id="user_birthday__1i" name="user_birthday[(1i)]">
    <option value="1998">Heisei 10</option>
    <option value="1999">Heisei 11</option>
    <option value="2000">Heisei 12</option>
    </select>
    /* The rest is omitted */

P.S.(on March 8)
Changed option name to labels_for_year_options. The argument is also changed from Array to Lambda. Example:

    date_select('user_birthday', '', start_year: 1998, end_year: 2000, labels_for_year_options: ->year { "Heisei #{ year - 1988 }" })

The same HTML as written above is produced.

P.S.(on March 9)
Changed option name again to label_for_year. Nothing has changed except it and code style since March 8.

P.S.(on March 15)
Changed option name again to year_format.

Other Information

In Japan, Wareki, the Japanese calender, is used as often as Western Calender. For example, 2000 A.D. is Heisei 12 in Wareki. It is even used in official documents. Therefore, there is a need for date select box with Wareki years. Some other countries like Israel and Thailand also have their own calenders, so I think the number of people who need this use_year_names option is not small.

rails-bot · 2018-03-07T15:27:37Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

yskkin · 2018-03-07T23:13:42Z

I also patched date_select to achieve this use case and found year_format_string: "Heisei %d" is better since we don't have to repeat "Heisei" and it is consistent with month_format_string option.

year_format_string: "%d年" (meaning "Year %d") or year_format_string: "FY %d" is another use case for this feature.

rafaelfranca · 2018-03-07T23:25:08Z

I agree with @yskkin. But if you need to mix different names it feels like you would be better served with an helper in your application.

yskkin · 2018-03-07T23:35:18Z

Ah, Western calendar and Japanese calendar are different so we can't handle with %d...
I was wrong but still feels that ["Heisei 10", "Heisei 11", "Heisei 12"] is not a right way.
Now I don't know what is appropriate API...

sikachu · 2018-03-08T01:20:56Z

Yeah, this is actually going to be tricky for Japanese year 😅

With Thai Buddhist calendar, we got a easy way out since at the end of the day it's just a numeric value:

> helper.select_year Date.today.year + 543, start_year: Date.today.year + 533, end_year: Date.today.year + 543
=> "<select id=\"date_year\" name=\"date[year]\">\n<option value=\"2551\">2551</option>\n<option value=\"2552\">2552</option>\n<option value=\"2553\">2553</option>\n<option value=\"2554\">2554</option>\n<option value=\"2555\">2555</option>\n<option value=\"2556\">2556</option>\n<option value=\"2557\">2557</option>\n<option value=\"2558\">2558</option>\n<option value=\"2559\">2559</option>\n<option value=\"2560\">2560</option>\n<option value=\"2561\" selected=\"selected\">2561</option>\n</select>\n"

For Japanese year, I feel like having to pass use_year_names still doesn't feel right to me since I would expect things to happen automatically instead (so if you do select_year Date.today, start_year: 1985 you'd get [Showa 60, Showa 61, Showa 62, Showa 63, Showa 64/Heisei 1, Heisei 2, ...] and so on automatically).

I dug around and actually found a wareki gem which helps conversion from Gregorian date. I have a feeling that there must be a market in need of, say, wareki-rails gem that would aid the date/time conversion. I also think that it's more appropriate to be in a separate gem as it's very Japan-specific feature (otherwise I'd already submit patch for Buddhist year for Thailand 😄)

liwii · 2018-03-08T07:44:03Z

@yskkin @rafaelfranca @sikachu
Thanks for your reviews!
Well, I understand what you say. I don't think that this use_year_names option is the best way to achieve this goal, either. So at first I thought about implementing this option using %d as @yskkin says, but I couldn't find a simple way to pass different number in %d and value of option tag, so I used array. It's the same way as use_month_names option, which already exists.
And as @sikachu says, it is true that Wareki is unique problem in Japan. However, I still think that rails needs an option like I proposed (though I don't stick to the format in this PR). In the case of Thai Buddhist calender, to store data properly, the HTML options should be like:

<option value="2008">2551</option>
<option value="2009">2552</option>
<option value="2010">2553</option>

But now year field in date_select doesn't have a way to use different number in option value and what is displayed. Since Western Calender is not the only format of year, I think rails at least needs a way to put different value in these two as in month field.

sikachu · 2018-03-08T08:08:25Z

Actually, one good way that we can achieve this is maybe letting user pass in a proc?

date_select('user_birthday', '', start_year: 1998, end_year: 2000, labels_for_year_options: ->{ |year| convert_year_to_wareki(year) })

liwii · 2018-03-08T08:15:58Z

It seems much better for me. I try to revise my codes in this way.

liwii · 2018-03-08T09:47:16Z

Adopting @sikachu 's suggestion, I changed my codes and now the option takes Lambda as the argument. If the change is approved, I will squash the commits and change my comment on the commit.

sikachu

There're just a few issues with the code style. I've left comments in-line.

After reading though it, I feel like labels_for_year_options actually a bit mouthful though. I think we can get away with label_for_year: -> {...} for brevity.

Nothing else has stood out for me, but if someone who's native English speaker can double check the documentation that'd be superb. I think this is a nice addition! 👍

sikachu · 2018-03-08T12:27:57Z

actionview/lib/action_view/helpers/date_helper.rb

@@ -846,12 +846,13 @@ def select_year
          options[:step]              = options[:start] < options[:end] ? 1 : -1
          options[:leading_zeros]     = false
          options[:max_years_allowed] = @options[:max_years_allowed] || 1000
+          options[:labels_for_year_options]    = @options[:labels_for_year_options]


I think this line was tagged by code climate. I think we stopped aligning operators, so you can just leave 1 space before =.

sikachu · 2018-03-08T12:29:11Z

actionview/lib/action_view/helpers/date_helper.rb

@@ -996,6 +997,40 @@ def build_options(selected, options = {})
          (select_options.join("\n") + "\n").html_safe
        end

+        # Build select option HTML for year.
+        # If <tt>use_year_names</tt> option is not passed,


Do you mind updating this row to reflect the new setting name?

sikachu · 2018-03-08T12:30:07Z

actionview/lib/action_view/helpers/date_helper.rb

+          end
+
+          (select_options.join("\n") + "\n").html_safe
+


Code style: Do you mind removing this empty blank line?

sikachu · 2018-03-08T12:30:16Z

actionview/lib/action_view/helpers/date_helper.rb

+
+        end
+
+


Code style: Do you mind removing this empty blank line too?

liwii · 2018-03-09T01:35:27Z

@sikachu
I adopted all your suggestion and changed my code style. Thanks for you reviews a lot! As you say, now I wait for some native speakers reviewing my documentation.

matthewd · 2018-03-12T02:03:06Z

I think there's room to more closely mirror the month handling:

I suggest we call the option :year_format, similar to the existing :month_format_string.
A year_name(number) method can take care of the lambda, and also fall back to number -- better to always use one code path instead of sometimes using build_options.

However, I'm a bit unsure whether it's worthwhile for us to support this... it's absolutely true that there are a number of other calendar systems, but this only helps the (small?) subset of those that still use Gregorian day/month numbering, and [at least approximately] agree that the year starts on Jan 1.

On balance I think it's probably okay -- we do already provide a lot of formatting options on this helper.

sikachu · 2018-03-15T10:34:36Z

Yeah, I feel like it's worthwhile to add support for this as well. And yeah, I agree with calling the option :year_format.

@liwii would you mind updating the PR per @matthewd's request and update your branch? I think it needs a rebase as well due to CHANGELOG conflict. Thanks!

liwii · 2018-03-15T14:56:41Z

@matthewd
Thanks for your review. I agree that your suggestion will make my new option more close to existing month handling. I will adopt your suggestion and update my branch soon.
@sikachu
Thanks for making comments a lot of times. Of course I don't mind adopting @matthewd's request and resolving conflicts. I will commit the change immediately.

liwii · 2018-03-15T15:24:16Z

@matthewd @sikachu
I adopted all your suggestions and squashed all the commits!

liwii · 2018-04-03T15:12:40Z

@sikachu @matthewd
I also resolved conflicts heppened during waiting the reviews. Do I need to do anything addtionally?

sikachu

Thank you for updating the patch!

@matthewd do you mind taking care of this forward? I think this is good to go.

liwii · 2018-06-10T11:50:07Z

I found needles options[:year_format] = @options[:label_for_year] in DatetimeSelector#select_year, so I removed it. Tracis CI started to fail, but the error doesn't seem relevant to my modification...

kamipo · 2018-06-10T11:58:20Z

CI failure has been addressed at 691addb.

kamipo · 2018-06-10T11:59:05Z

actionview/lib/action_view/helpers/date_helper.rb

@@ -933,6 +933,21 @@ def month_name(number)
          end
        end

+        # Looks up year names by number.
+        #
+        #   month_name(1998) # => 1998


year_name(1998)

kamipo · 2018-06-10T11:59:19Z

actionview/lib/action_view/helpers/date_helper.rb

+        #
+        # If the <tt>:year_format</tt> option is passed:
+        #
+        #   month_name(1998) # => "Heisei 10"


year_name(1998)

…sible to customize year names. Lambda should be passed to use this option. Example: date_select('user_birthday', '', start_year: 1998, end_year: 2000, year_format: ->year { "Heisei #{year - 1988}" }) The HTML produced: <select id="user_birthday__1i" name="user_birthday[(1i)]"> <option value="1998">Heisei 10</option> <option value="1999">Heisei 11</option> <option value="2000">Heisei 12</option> </select> /* The rest is omitted */

liwii · 2018-06-10T12:10:38Z

@kamipo
Thanks for your review! Now I took 691addb into my branch so maybe CI problem is resolved.
I also fixed the typo you pointed out.

sikachu · 2018-06-21T01:11:52Z

@kamipo do you have any further feedback? Is this ready to be merged in?

kamipo · 2018-06-21T07:28:39Z

Yeah, I think this is ready to be merged in.

sikachu · 2018-06-21T07:31:28Z

@kamipo would you mind merging this in? (I'm in issues team so I don't have permission to merge 🙇)

Add `use_year_names` option to date_select tag

Follow up rails#32190 [ci skip]

rails-bot assigned kaspth Mar 7, 2018

liwii changed the title ~~Add use_year_name option to date_select tag~~ Add use_year_names option to date_select tag Mar 8, 2018

sikachu requested changes Mar 8, 2018

View reviewed changes

liwii force-pushed the use_year_names branch from 4bb0432 to 8f240f4 Compare March 15, 2018 15:23

liwii force-pushed the use_year_names branch from 8f240f4 to 511e8af Compare March 16, 2018 14:18

liwii force-pushed the use_year_names branch from 511e8af to 54f5e06 Compare April 3, 2018 15:11

liwii force-pushed the use_year_names branch from 54f5e06 to 6784a38 Compare May 27, 2018 15:31

sikachu approved these changes Jun 8, 2018

View reviewed changes

liwii force-pushed the use_year_names branch from 6784a38 to e1036d0 Compare June 10, 2018 09:18

liwii force-pushed the use_year_names branch from c19de3d to e1036d0 Compare June 10, 2018 11:56

liwii force-pushed the use_year_names branch from e1036d0 to ebb0e85 Compare June 10, 2018 11:59

kamipo reviewed Jun 10, 2018

View reviewed changes

liwii force-pushed the use_year_names branch from ebb0e85 to 8f46a23 Compare June 10, 2018 12:08

kamipo approved these changes Jun 21, 2018

View reviewed changes

kamipo merged commit 8f46a23 into rails:master Jun 21, 2018

kamipo added a commit that referenced this pull request Jun 21, 2018

Merge pull request #32190 from liwii/use_year_names

8f6c85f

Add `use_year_names` option to date_select tag

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Jun 21, 2018

Add to docs mention about :year_format option of date select

04f9b8f

Follow up rails#32190 [ci skip]

bogdanvlviv mentioned this pull request Jun 21, 2018

Add to docs mention about :year_format option of date select #33184

Merged

kamipo mentioned this pull request Oct 29, 2021

Add :day_format option to date_select #43567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `use_year_names` option to date_select tag #32190

Add `use_year_names` option to date_select tag #32190

liwii commented Mar 7, 2018 •

edited

rails-bot commented Mar 7, 2018

yskkin commented Mar 7, 2018

rafaelfranca commented Mar 7, 2018

yskkin commented Mar 7, 2018

sikachu commented Mar 8, 2018

liwii commented Mar 8, 2018

sikachu commented Mar 8, 2018 •

edited

liwii commented Mar 8, 2018

liwii commented Mar 8, 2018

sikachu left a comment

sikachu Mar 8, 2018

sikachu Mar 8, 2018

sikachu Mar 8, 2018

sikachu Mar 8, 2018

liwii commented Mar 9, 2018

matthewd commented Mar 12, 2018

sikachu commented Mar 15, 2018

liwii commented Mar 15, 2018

liwii commented Mar 15, 2018

liwii commented Apr 3, 2018

sikachu left a comment

liwii commented Jun 10, 2018

kamipo commented Jun 10, 2018

kamipo Jun 10, 2018

kamipo Jun 10, 2018

liwii commented Jun 10, 2018

sikachu commented Jun 21, 2018

kamipo commented Jun 21, 2018

sikachu commented Jun 21, 2018

Add use_year_names option to date_select tag #32190

Add use_year_names option to date_select tag #32190

Conversation

liwii commented Mar 7, 2018 • edited

Summary

Other Information

rails-bot commented Mar 7, 2018

yskkin commented Mar 7, 2018

rafaelfranca commented Mar 7, 2018

yskkin commented Mar 7, 2018

sikachu commented Mar 8, 2018

liwii commented Mar 8, 2018

sikachu commented Mar 8, 2018 • edited

liwii commented Mar 8, 2018

liwii commented Mar 8, 2018

sikachu left a comment

Choose a reason for hiding this comment

sikachu Mar 8, 2018

Choose a reason for hiding this comment

sikachu Mar 8, 2018

Choose a reason for hiding this comment

sikachu Mar 8, 2018

Choose a reason for hiding this comment

sikachu Mar 8, 2018

Choose a reason for hiding this comment

liwii commented Mar 9, 2018

matthewd commented Mar 12, 2018

sikachu commented Mar 15, 2018

liwii commented Mar 15, 2018

liwii commented Mar 15, 2018

liwii commented Apr 3, 2018

sikachu left a comment

Choose a reason for hiding this comment

liwii commented Jun 10, 2018

kamipo commented Jun 10, 2018

kamipo Jun 10, 2018

Choose a reason for hiding this comment

kamipo Jun 10, 2018

Choose a reason for hiding this comment

liwii commented Jun 10, 2018

sikachu commented Jun 21, 2018

kamipo commented Jun 21, 2018

sikachu commented Jun 21, 2018

Add `use_year_names` option to date_select tag #32190

Add `use_year_names` option to date_select tag #32190

liwii commented Mar 7, 2018 •

edited

sikachu commented Mar 8, 2018 •

edited