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

Added overlapEventStartOnly prop #182

Merged
merged 3 commits into from Oct 16, 2019
Merged

Added overlapEventStartOnly prop #182

merged 3 commits into from Oct 16, 2019

Conversation

ghost
Copy link

@ghost ghost commented Oct 9, 2019

Hi Antoni,
Got another PR here with a feature that may not see a lot of use, but works well with my kind of events data.

Goal

Reduce wasted horizontal space in cells with many overlapping events by letting them fall under events starting later leveraging the existing z-index structure.
chrome_ary3VwRcUd

Problem

Event width is based on the overlaps property which is calculated using the entire length of events to check if they overlap at any time, but what if we don't need it to be so thorough and are okay to :hover events to see them in full.

Solution

Introducing the overlapEventStartOnly (please feel free to rename vars to whatever you like) vue-cal property. When enabled bypasses eventInRange(...) in lieu of a simple start time comparison. Leading to a view like:
image
And of course the partially covered events come forward to full glory when you hit them with the cursor:
image

@antoniandre
Copy link
Owner

Thanks again for your contribution @korpow.
I think I'll accept that in, today or tomorrow, but I'm still wondering if this is not an edge case.

I try not to add edge cases in the code because every feature we add is another layer of complexity in the codebase and makes it harder to maintain, so I am always picky about what I am adding. 😅

I'm wondering how does this look in the case where the events are not starting at the same time but less than 30min apart with a time scale of 30min or 1h.

@ghost
Copy link
Author

ghost commented Oct 16, 2019

@antoniandre it is true the feature could see low adoption, and nice catch I need to work on my QA 😅 the scenario you describe is not ideal and I am sure if you make 2 events 1 minute apart hilarity ensues with overlapEventStartOnly enabled.
image

With the aid of a new helper function datesInSameTimestep(date1, date2, timeStep) the "overlapping starts" are now in the context of any event that starts in the same time slot according to the timeStep prop. Avoiding making drastic changes to event-utils is my goal, but getting at that overlap logic is proving quite tough.

So now it comes out looking like this:
image

@antoniandre
Copy link
Owner

Hi @korpow,
Thanks for testing the case and taking screenshots.
I'm glad my remark teased you and you made a great improvement, haha.

but getting at that overlap logic is proving quite tough.

Yes, this is very true, what I've been wanting to do for instance, is to place events in a way to optimise the cell space as smartly as possible, take this example from the doc:

image

When I release the orange event here, ideally the green event will go to the right and the blue and red events will go to the left, all of them would have a width of 50%.

Easy to see that it's better like that, but not easy to create an algorithm for that without hard coded cases!
I ended up with what we have now, not perfect but good enough. 😅

Anyway, I'll merge this soon. Thanks for your work mate.

@ghost
Copy link
Author

ghost commented Oct 16, 2019

Totally agree with you on what the ideal result of your screenshot would be and the challenges.
For now I'm perfectly happy with the results of the last 2 PRs to deploy vue-cal in my project.

Thanks for continuing to improve this component!

@antoniandre antoniandre merged commit b997b8e into antoniandre:master Oct 16, 2019
@antoniandre
Copy link
Owner

Hi @korpow, that's now published in latest version!
btw, I renamed the feature to overlapsPerTimeStep.

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.

None yet

1 participant