Fix typos and remove unused options in ActionView::Helpers::DateTimeSelector #43566
Conversation
| @@ -1012,14 +1012,14 @@ def build_options(selected, options = {}) | |||
| end | |||
|
|
|||
| # Build select option HTML for year. | |||
| # If <tt>year_format</tt> option is not passed | |||
| # If <tt>year_format</tt> option is not passed to DateTimeSelector | |||
There was a problem hiding this comment.
Please remove this change since it doesn't make sense. We don't pass this option to a module. The example already explain to who we pass it.
There was a problem hiding this comment.
I reverted the change.
I think you mean the comment implies the option is passed to date_select or datetime_select. Is it correct?
| # build_year_options(1998, start: 1998, end: 2000) | ||
| # => "<option value="1998" selected="selected">1998</option> | ||
| # <option value="1999">1999</option> | ||
| # <option value="2000">2000</option>" | ||
| # | ||
| # If <tt>year_format</tt> option is passed | ||
| # build_year_options(1998, start: 1998, end: 2000, year_format: ->year { "Heisei #{ year - 1988 }" }) | ||
| # If <tt>year_format: ->year { "Heisei #{ year - 1988 }</tt> option is passed to DateTimeSelector |
There was a problem hiding this comment.
Also remove this change. The example should be enough.
There was a problem hiding this comment.
I reverted the change.
| @@ -938,7 +938,7 @@ def month_name(number) | |||
| # | |||
| # year_name(1998) # => 1998 | |||
| # | |||
| # If the <tt>:year_format</tt> option is passed: | |||
| # If the <tt>year_format: ->(year) { "Heisei #{year - 1988}" }</tt> option is passed: | |||
There was a problem hiding this comment.
Please remove this from here and add to the example
There was a problem hiding this comment.
I reverted the change.
| @@ -1019,7 +1019,7 @@ def build_options(selected, options = {}) | |||
| # <option value="2000">2000</option>" | |||
| # | |||
| # If <tt>year_format</tt> option is passed | |||
| # build_year_options(1998, start: 1998, end: 2000, year_format: ->year { "Heisei #{ year - 1988 }" }) | |||
| # build_year_options(1998, start: 1998, end: 2000) | |||
| # => "<option value="1998" selected="selected">Heisei 10</option> | |||
There was a problem hiding this comment.
I think year_format option should not be passed to build_year_options. it doesn't deal with the option.
There was a problem hiding this comment.
True. In that case let's remove it.
There was a problem hiding this comment.
@rafaelfranca @shunichi Looks like this whole piece of doc is no longer relevant, lines 1021-1023 ?
Otherwise this PR looks pretty good to me. Can I help in some way to move it forward?
There was a problem hiding this comment.
This comment is so confusing.
In the comment, option is not build_year_options' argument but DateTimeSelector.new's.
Other methods likemonth_name have similar comments. But they have no arguments named options. So their comments are less confusing.
I removed the comment about year_format at 0c53554843bc51fbfa86c3678bd9cf5e741d6da9
|
@rafaelfranca |
|
@shunichi Looks like you need to rebase from main? The buildkite errors don't seem to have anything to do with your PR. |
shunichi
force-pushed
the
fix-typos-and-remove-unused-options
branch
from
0c53554
to
a692bb6
Compare
|
@pjfitzgibbons I rebased and squashed commits. |
|
Looks great! Still failing buildkite... in ActiveJob though. Are you rebased w/ rails:main ? |
Yes, I rebased onto 23ce9a7 which is rails:main HEAD at the time of rebase. |
Summary
This fixes following:
ActionView::Helpers::DateTimeSelectorhas some wrong comments.ActionView::Helpers::DateTimeSelector#select_yearhas some unused code.No behavior changed.
Other Information
No other information.