Add `beginning_of_week` option to `weekday_options_for_select` #43037

DRBragg · 2021-08-17T16:07:49Z

Summary

In #42979 the suggestion was made to include an option to allow users to set "Monday" as the first day on the list.

weekday_options_for_select(begin_on_monday: true)
# => "<option value=\"Monday\">Monday</option>\n<option value=\"Tuesday\">Tuesday</option>\n
# <option value=\"Wednesday\">Wednesday</option>\n<option value=\"Thursday\">Thursday</option>\n
# <option value=\"Friday\">Friday</option>\n<option value=\"Saturday\">Saturday</option>\n
# <option value=\"Sunday\">Sunday</option>"

NOTE: If you are using indexes as values Sundays value will remain 0 for consistency with Ruby and the I18n day names array.

weekday_options_for_select(begin_on_monday: true, index_as_value: true)
# => "<option value=\"1\">Monday</option>\n<option value=\"2\">Tuesday</option>\n
# <option value=\"3\">Wednesday</option>\n<option value=\"4\">Thursday</option>\n
# <option value=\"5\">Friday</option>\n<option value=\"6\">Saturday</option>\n
# <option value=\"0\">Sunday</option>"

p8 · 2021-08-17T16:30:00Z

In Active Support Date has a beginning_of_week method.
There also is Rails.application.config.beginning_of_week.
Maybe the option should be beginning_of_week that defaults to Rails.application.config.beginning_of_week?
That would allow beginning on saturday as well.

DRBragg · 2021-08-17T17:37:11Z

In Active Support Date has a beginning_of_week method.
There also is Rails.application.config.beginning_of_week.
Maybe the option should be beginning_of_week that defaults to Rails.application.config.beginning_of_week?
That would allow beginning on saturday as well.

Yea, I had thought about using that but thought it wasn't worth having to handle all the days of the week. Maybe that isn't an issue and we should just handle/allow for any day? I just wasn't sure why you would want to do that but I also didn't think about Saturday.

DRBragg · 2021-08-19T15:08:57Z

@p8 What would you think about only handling :sunday, :monday, or :saturday as beginning_of_week options since it seems those are the only options world wide? We'll make it so it defaults to Rails.application.config.beginning_of_week but if it's any day but the aforementioned we basically ignore it? Or do you think it's worth handling any and all days?

Here's basically what I'm thinking:

def weekday_options_for_select(selected = nil, index_as_value: false, day_format: :day_names, beginning_of_week: Date.beginning_of_week)
  rotations = { sunday: 0, monday: 1, saturday: -1 }
  day_names = I18n.translate("date.#{day_format}")
  day_names = day_names.map.with_index.to_a if index_as_value
  day_names = day_names.rotate(rotations.fetch(beginning_of_week, 1))

  options_for_select(day_names, selected)
end

Or something like that

DRBragg · 2021-08-20T20:27:39Z

@p8 I made the change, let me know what you think.

actionview/lib/action_view/helpers/form_options_helper.rb

DRBragg · 2021-09-08T22:27:14Z

Ping or bump (not sure what the SOP is here)

kaspth

Sweet, gotta have the commits squashed too! The test failures look unrelated.

kaspth · 2021-09-17T23:30:41Z

actionview/lib/action_view/helpers/form_options_helper.rb

+      # * <tt>:beginning_of_week</tt> - Defaults to Date.beginning_of_week, accepts
+      # :sunday, :monday, or :saturday values to set the first day on the list.


We support all days now, and can just omit these specific references:

Suggested change

# * <tt>:beginning_of_week</tt> - Defaults to Date.beginning_of_week, accepts

# :sunday, :monday, or :saturday values to set the first day on the list.

# * <tt>:beginning_of_week</tt> - Defaults to Date.beginning_of_week.

Change `begin_on_monday` to `beginning_of_week` Change `begin_on_monday` to `beginning_of_week` Better handle ignored day Remove inaccurate tests Improve docs and use DAYS_INTO_WEEK to calc rotation Remove inaccurate docs

kaspth · 2021-09-17T23:47:57Z

The Action View builds passed and the failures were on main in components that this didn't touch.

Thanks so much @DRBragg, dig how this turned out 🙌

DRBragg · 2021-09-17T23:50:03Z

The Action View builds passed and the failures were on main in components that this didn't touch.

Thanks so much @DRBragg, dig how this turned out 🙌

Thank YOU so much @kaspth, for all the great feedback and for sticking with me through this! You rock dude, so so much!

zzak · 2021-09-26T01:06:22Z

actionview/test/template/form_options_helper_test.rb

      weekday_options_for_select
    )
  end

  def test_weekday_options_for_select_with_index_as_value
    assert_dom_equal(
-      "<option value=\"0\">Sunday</option>\n<option value=\"1\">Monday</option>\n<option value=\"2\">Tuesday</option>\n<option value=\"3\">Wednesday</option>\n<option value=\"4\">Thursday</option>\n<option value=\"5\">Friday</option>\n<option value=\"6\">Saturday</option>",
+      "<option value=\"1\">Monday</option>\n<option value=\"2\">Tuesday</option>\n<option value=\"3\">Wednesday</option>\n<option value=\"4\">Thursday</option>\n<option value=\"5\">Friday</option>\n<option value=\"6\">Saturday</option>\n<option value=\"0\">Sunday</option>",


@DRBragg @kaspth I find it odd that the default behavior changed to use Monday as the start of the week, but the index value still starts at 0 with Sunday 🤔

It's is a little odd but since Date.beginning_of_week defaults to Monday in Rails it's what works best in a Rails context. If you set config.beginning_of_week = :sunday in your config file this will default to Sunday instead

@DRBragg Right, but if Date.beginning_of_week defaults to Monday, that explains why "Monday" is the first <option> in the select but doesn't explain why "Sunday" has the 0 index, and "Monday" is 1. Is it a bug? 🤔

Sunday is 0 because I18n.translate("date.day_names") returns an array where Sunday is the first item, Date::DAYS_INTO_WEEK returns a hash where the value of the key :sunday is 0, and in Ruby calling wday on a date that is a Sunday will return 0. I'm not 100% why Date.beginning_of_week defaults to Monday

rails-bot bot added the actionview label Aug 17, 2021

rails-bot bot added actionpack activejob activerecord activesupport docs labels Aug 20, 2021

DRBragg force-pushed the drbragg/update_weekday_select branch from 2873ea7 to 9694317 Compare August 20, 2021 20:24

DRBragg force-pushed the drbragg/update_weekday_select branch from 11c5e62 to 8400ee8 Compare August 20, 2021 20:31

DRBragg mentioned this pull request Aug 21, 2021

Add weekday_options_for_select method #42979

Merged

DRBragg changed the title ~~Add begin_on_monday option to weekday_options_for_select~~ Add beginning_of_week option to weekday_options_for_select Aug 21, 2021

kaspth reviewed Aug 23, 2021

View reviewed changes

actionview/lib/action_view/helpers/form_options_helper.rb Outdated Show resolved Hide resolved

actionview/lib/action_view/helpers/form_options_helper.rb Outdated Show resolved Hide resolved

kaspth approved these changes Sep 17, 2021

View reviewed changes

Add beginning_of_week option to weekday_options_for_select

451c55e

Change `begin_on_monday` to `beginning_of_week` Change `begin_on_monday` to `beginning_of_week` Better handle ignored day Remove inaccurate tests Improve docs and use DAYS_INTO_WEEK to calc rotation Remove inaccurate docs

DRBragg force-pushed the drbragg/update_weekday_select branch from aa0a540 to 451c55e Compare September 17, 2021 23:40

kaspth merged commit f2209e9 into rails:main Sep 17, 2021

zzak reviewed Sep 26, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add `beginning_of_week` option to `weekday_options_for_select` #43037

Add `beginning_of_week` option to `weekday_options_for_select` #43037

Uh oh!

DRBragg commented Aug 17, 2021

Uh oh!

p8 commented Aug 17, 2021 •

edited

Loading

Uh oh!

DRBragg commented Aug 17, 2021

Uh oh!

DRBragg commented Aug 19, 2021 •

edited

Loading

Uh oh!

DRBragg commented Aug 20, 2021

Uh oh!

Uh oh!

Uh oh!

DRBragg commented Sep 8, 2021

Uh oh!

kaspth left a comment

Uh oh!

kaspth Sep 17, 2021

Uh oh!

DRBragg Sep 17, 2021

Uh oh!

kaspth commented Sep 17, 2021

Uh oh!

DRBragg commented Sep 17, 2021

Uh oh!

zzak Sep 26, 2021

Uh oh!

DRBragg Sep 26, 2021

Uh oh!

zzak Sep 26, 2021

Uh oh!

DRBragg Sep 27, 2021

Uh oh!

Uh oh!

		# * <tt>:beginning_of_week</tt> - Defaults to Date.beginning_of_week, accepts
		# :sunday, :monday, or :saturday values to set the first day on the list.

Add beginning_of_week option to weekday_options_for_select #43037

Add beginning_of_week option to weekday_options_for_select #43037

Uh oh!

Conversation

DRBragg commented Aug 17, 2021

Summary

Uh oh!

p8 commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DRBragg commented Aug 17, 2021

Uh oh!

DRBragg commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DRBragg commented Aug 20, 2021

Uh oh!

Uh oh!

Uh oh!

DRBragg commented Sep 8, 2021

Uh oh!

kaspth left a comment

Choose a reason for hiding this comment

Uh oh!

kaspth Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

DRBragg Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

kaspth commented Sep 17, 2021

Uh oh!

DRBragg commented Sep 17, 2021

Uh oh!

zzak Sep 26, 2021

Choose a reason for hiding this comment

Uh oh!

DRBragg Sep 26, 2021

Choose a reason for hiding this comment

Uh oh!

zzak Sep 26, 2021

Choose a reason for hiding this comment

Uh oh!

DRBragg Sep 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add `beginning_of_week` option to `weekday_options_for_select` #43037

Add `beginning_of_week` option to `weekday_options_for_select` #43037

p8 commented Aug 17, 2021 •

edited

Loading

DRBragg commented Aug 19, 2021 •

edited

Loading