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

Add `use_year_names` option to date_select tag #32190

Merged
merged 1 commit into from Jun 21, 2018

Conversation

Projects
None yet
8 participants
@liwii
Copy link
Contributor

liwii commented Mar 7, 2018

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

This comment has been minimized.

Copy link

rails-bot commented Mar 7, 2018

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

This comment has been minimized.

Copy link
Contributor

yskkin commented Mar 7, 2018

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

This comment has been minimized.

Copy link
Member

rafaelfranca commented Mar 7, 2018

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

This comment has been minimized.

Copy link
Contributor

yskkin commented Mar 7, 2018

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

This comment has been minimized.

Copy link
Member

sikachu commented Mar 8, 2018

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

This comment has been minimized.

Copy link
Contributor Author

liwii commented Mar 8, 2018

@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

This comment has been minimized.

Copy link
Member

sikachu commented Mar 8, 2018

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

@liwii

This comment has been minimized.

Copy link
Contributor Author

liwii commented Mar 8, 2018

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

@liwii

This comment has been minimized.

Copy link
Contributor Author

liwii commented Mar 8, 2018

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
Copy link
Member

sikachu left a comment

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! 👍

actionview/lib/action_view/helpers/date_helper.rb Outdated
@@ -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]

This comment has been minimized.

Copy link
@sikachu

sikachu Mar 8, 2018

Member

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

actionview/lib/action_view/helpers/date_helper.rb Outdated
@@ -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,

This comment has been minimized.

Copy link
@sikachu

sikachu Mar 8, 2018

Member

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

actionview/lib/action_view/helpers/date_helper.rb Outdated
end

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

This comment has been minimized.

Copy link
@sikachu

sikachu Mar 8, 2018

Member

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

actionview/lib/action_view/helpers/date_helper.rb Outdated

end


This comment has been minimized.

Copy link
@sikachu

sikachu Mar 8, 2018

Member

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

@liwii

This comment has been minimized.

Copy link
Contributor Author

liwii commented Mar 9, 2018

@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

This comment has been minimized.

Copy link
Member

matthewd commented Mar 12, 2018

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

This comment has been minimized.

Copy link
Member

sikachu commented Mar 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

liwii commented Mar 15, 2018

@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 liwii force-pushed the liwii:use_year_names branch Mar 15, 2018

@liwii

This comment has been minimized.

Copy link
Contributor Author

liwii commented Mar 15, 2018

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

@liwii liwii force-pushed the liwii:use_year_names branch Mar 16, 2018

@liwii liwii force-pushed the liwii:use_year_names branch Apr 3, 2018

@liwii

This comment has been minimized.

Copy link
Contributor Author

liwii commented Apr 3, 2018

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

@liwii liwii force-pushed the liwii:use_year_names branch May 27, 2018

@sikachu

sikachu approved these changes Jun 8, 2018

Copy link
Member

sikachu left a comment

Thank you for updating the patch!

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

@liwii liwii force-pushed the liwii:use_year_names branch Jun 10, 2018

@liwii

This comment has been minimized.

Copy link
Contributor Author

liwii commented Jun 10, 2018

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

@liwii liwii force-pushed the liwii:use_year_names branch Jun 10, 2018

@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Jun 10, 2018

CI failure has been addressed at 691addb.

@liwii liwii force-pushed the liwii:use_year_names branch Jun 10, 2018

actionview/lib/action_view/helpers/date_helper.rb Outdated
@@ -933,6 +933,21 @@ def month_name(number)
end
end

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

This comment has been minimized.

Copy link
@kamipo

kamipo Jun 10, 2018

Member

year_name(1998)

actionview/lib/action_view/helpers/date_helper.rb Outdated
#
# If the <tt>:year_format</tt> option is passed:
#
# month_name(1998) # => "Heisei 10"

This comment has been minimized.

Copy link
@kamipo

kamipo Jun 10, 2018

Member

year_name(1998)

Add `year_format` option to date_select tag. This option makes it pos…
…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 liwii force-pushed the liwii:use_year_names branch to 8f46a23 Jun 10, 2018

@liwii

This comment has been minimized.

Copy link
Contributor Author

liwii commented Jun 10, 2018

@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

This comment has been minimized.

Copy link
Member

sikachu commented Jun 21, 2018

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

@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Jun 21, 2018

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

@kamipo

kamipo approved these changes Jun 21, 2018

@sikachu

This comment has been minimized.

Copy link
Member

sikachu commented Jun 21, 2018

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

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

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Merge pull request #32190 from liwii/use_year_names
Add `use_year_names` option to date_select tag

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.