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.

Sign up for GitHub

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

Jump to bottom

date_select helper method extension #13591

Closed
wants to merge 1 commit into from
Closed

date_select helper method extension #13591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions actionview/lib/action_view/helpers/date_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,19 +817,25 @@ def select_month
end

def select_year
middle_years = [Date.today.year]

if !@datetime || @datetime == 0
val = '1'
middle_year = Date.today.year
else
val = middle_year = year
val = year
if year.abs === Float::INFINITY
Copy link
Member

Choose a reason for hiding this comment

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

Why this infinity case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checkout this test case, this is for infinity.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. but why it is needed only now?

middle_years = [year]
else
middle_years.push(year)
end
end

if @options[:use_hidden] || @options[:discard_year]
build_hidden(:year, val)
else
options = {}
options[:start] = @options[:start_year] || middle_year - 5
options[:end] = @options[:end_year] || middle_year + 5
options[:start] = @options[:start_year] || middle_years.min - 5
Copy link
Member

Choose a reason for hiding this comment

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

Why this min case? Lets take the simpler and safer path and change this code to take in consideration only the Data.today.year.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this is more safer 👍 I will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rafaelfranca But suppose a user selected 1905 as selected date, then when we will try to edit the field we won't have the option in the DropDown list because DropDown menu will start from Date.today.year. So, i think we should go with this approach. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

How users will select 1905 if it is not disponible in the select box?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e.g.
<%= f.date_select :dob %>, and say I choose my DOB to Date.today.year - 5, now I came after 1 year to edit my DOB, so now the DropDown list won't contain my previously selected option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current system, the start and end limits are also dependent on the selected value. Am I wrong in this?

Copy link
Member

Choose a reason for hiding this comment

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

No, you are right. This is exactly what is causing confusion. If we are going to keep the implementation dependent on the selected value I prefer to keep it as it is since it is easier to explain than:

If there is a selected value the date_select helper will check if the selected value is lower than the current year and will calculate the start value subtracting 5 years.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rafaelfranca But there is an issue in that case, suppose we choose dob to Date.today.year - 5 and tried to save the object via form-submission and now because of some validation failure object does not save, now the user will have new list in the DropDown options. which wont have dates greater than Date.today.year.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm aware of this issue. What I'm trying to explain here is: we will not solve this problem adding more magic in this method. Adding more magic in the method will only add more confusion to users. After this "Why is the select taking in consideration the selected value?" is not a simple answer anymore.

For the issue you mentioned above I'd say, define the end value. If you think better if a validation error occurs hardly users will change the date.

I want to go simpler, or we change this behavior to do less magic or we keep it as it is. More magic is not an option here since it will only add more confusion.

Right now, I'm inclined to keep it as it is, since it will not require any change in the people application and the current behavior is documented.

Copy link
Member

Choose a reason for hiding this comment

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

I'll think better about this and do some tests with this branch.

Thank you for working on this ❤️

options[:end] = @options[:end_year] || middle_years.max + 5
options[:step] = options[:start] < options[:end] ? 1 : -1
options[:leading_zeros] = false
options[:max_years_allowed] = @options[:max_years_allowed] || 1000
Expand Down
Loading