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

Repeating monthly by day of week - include 5th week and LAST in UI #66

Closed
wants to merge 5 commits into from

Conversation

naomiaro
Copy link
Contributor

@naomiaro naomiaro commented Oct 8, 2014

currently there's only the possibility of choosing week 1-4 in the UI, losing some functionality from the ice cube library this way.

-every month on the first and last tuesdays of the month
schedule.add_recurrence_rule Rule.monthly.day_of_week(:tuesday => [1, -1])

@nathany
Copy link
Contributor

nathany commented Oct 14, 2014

Thanks for looking into this omission Naomi. I'm curious what the behaviour is when choosing 5th week of the month if the next month is only 4 weeks long? I've yet to test it out.

The test failures appear to be some unrelated sprockets thing. Sorry about that. We'll get that fixed in an independent pull request.

@naomiaro
Copy link
Contributor Author

Reading through the Ice cube docs briefly I think if a month doesn't have a 5th monday an event is just not created for that month. I'd have to double check that though.

@nathany
Copy link
Contributor

nathany commented Oct 14, 2014

That would be consistent with what I've seen for schedules like the 30th of the month. February just gets skipped.

@nathany
Copy link
Contributor

nathany commented Oct 14, 2014

If you merge master into your branch, the tests are passing now.

@nathany
Copy link
Contributor

nathany commented Oct 14, 2014

repeat

I also wonder what ice cube does when the choosing something like "Last Monday and 5th Monday".

@nathany
Copy link
Contributor

nathany commented Oct 14, 2014

@naomiaro Just shared this with our customer support and sales teams at Jobber. They said we've never had a request for 5th week and find the addition a bit confusing.

Perhaps it could be configurable? I'm not sure what others using recurring_select would like to see. Possibly some would like 4 weeks + last week without the 5th week row.

@naomiaro
Copy link
Contributor Author

Yes, configuring what to display would be good. To make it the most general I think I'd want to try to do some kind of matrix (if I was doing this for myself) week x day of week, so that perhaps businesses maybe wanting to use this calendar could display it without certain days of the week (sunday/saturday) or I know of businesses always closed on the 2nd and 4th monday of a month. Not sure if this would be too much for the project though, maybe just configuring the 5th week could be fine.

@nathany
Copy link
Contributor

nathany commented Oct 14, 2014

I think two options to configure the visibility of the 4th/5th and last week would be sufficient for now. The default could be the current behaviour, with some info in the REAMDE to enable the options.

Whether or not to make weekends available could be another option, but that can come later.

@naomiaro
Copy link
Contributor Author

OK, sounds good to me.

Including changes to make the build pass
…Hiding 5th and Last week by default to match current functionality.
@naomiaro
Copy link
Contributor Author

I've merged the upstream changes and provided some configurable defaults to show the weeks (updated the README as well.) Default functionality should be what the widget currently shows (4 weeks)

@nathany
Copy link
Contributor

nathany commented Oct 15, 2014

Thanks Naomi.

I'm not sure about doing the show_week config as part of the locale. Instead of using texts, what do you think about adding $.fn.recurring_select.options?

@naomiaro
Copy link
Contributor Author

Oh right, yes that would be way better. $.fn.recurring_select.options currently doesn't exist correct? I should create that.. maybe under options.monthly.week.show_week?

@nathany
Copy link
Contributor

nathany commented Oct 15, 2014

How about options.monthly.show_weeks?

…Hiding 5th and Last week by default to match current functionality.
Conflicts:
	README.md
	app/assets/javascripts/recurring_select_dialog.js.coffee.erb
@naomiaro
Copy link
Contributor Author

I've put those changes in the branch now

@nathany
Copy link
Contributor

nathany commented Oct 17, 2014

Thanks Naomi.

I'm not in love with the true, true, false, false way of configuring this. But I'm not sure how to do it better.

Let's go with this for now.

@nathany
Copy link
Contributor

nathany commented Oct 17, 2014

Now I see where you were going with options.monthly.week.show_week now. That could've been better, just seemed repetitive with "week, week".

@naomiaro
Copy link
Contributor Author

Yeah just thought it might be good to scope to each widget pane

@nathany
Copy link
Contributor

nathany commented Oct 17, 2014

Feel free to open a new pull request if you come up with a cleaner way to configure the options. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants