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

Adds compatibility for time_select helper and fixes "overlaps?" for certain cases #24

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kennyeni
Copy link
Contributor

Hi,

This fixes might be specific for my project, but I'm sharing them in case they are helpful for the community.

  1. time_select compatibility: rails has a helper method that renders a time selector on views, however, the object must have Date and Time attributes (although it does ignore the Date attributes...), so this little change adds a placeholder day that will be ignored AFAIK.
  2. overlaps?: as I understood this method, it should return a boolean that represents if two "Shifts" overlap in time, I found some cases that failed according to my logic, for example, calling
shift1 = Tod::Shift.new(Tod::TimeOfDay.new(7), Tod::TimeOfDay.new(2))
shift2 = Tod::Shift.new(Tod::TimeOfDay.new(1), Tod::TimeOfDay.new(4))

would represent something like this:
timeline

And overlaps? should return true AFAIK, which it does not.

I added a few more test cases on tests, as there are some edge cases to be considered.

One more time, I don't know if this is the expected behavior for all projects, and how the author planned them, but this proved to be useful in my particular case

kennyeni added 4 commits May 19, 2015 22:02
TOD currently works with time_field out of the box, but this HTML field
is not supported in all browsers, so this adds compatibility for a
helper method that generates standard HTML5 fields
@jackc
Copy link
Owner

jackc commented Jun 8, 2015

I merged in the overlaps changes, thanks!

I didn't merge in the time_select compatibility. I think you can use the time_select option :ignore_date instead.

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