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

fix overlapping background events, fixes #2452 #2510

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

chazzlabs
Copy link
Contributor

This PR addresses two issues with how background events are rendered using both the overlap and no-overlap layout algorithms. I opted for the change I made due to its simplicity and the unlikelihood that it would have any adverse effects elsewhere in the calendar. It appears to me that the implementation of the no-overlap layout algorithm very deliberately returns different types for the width and xOffset style values than does the overlap algorithm; given this it seemed most reasonable to correct how those values are handled when rendering events in TimeGridEvent.

I added two stories under "Additional Examples > Layout" to demonstrate the overlap/no-overlap behavior since that seemed like the most appropriate location for them.

@chazzlabs chazzlabs changed the title fix overlapping background events #2452 fix overlapping background events, fixes #2452 Feb 15, 2024
@cutterbl
Copy link
Collaborator

Thank You for contributing to Big Calendar! This is good stuff, but I do want to point out a few things I'm seeing.

Concerning the general layout of the 'no-overlap' algorithm with background events: this is 'no-overlap', as provided to standard events
Screenshot 2024-02-16 at 10 22 26 AM
While this is the new 'no-overlap' as applied to background events
Screenshot 2024-02-16 at 10 23 19 AM
There are two differences to notice here.

The first is that the background events actually do overlap, if only slightly, as opposed to the standard events, which have a small gap.

The second is the overall width of the background events. Notice that they extend all the way to the edge of the column? The 'gap' we've had is intentional, as it allows for slot selection without firing the 'onSelectEvent' (yes, background events will also fire this on click).

Really like where you're going here. Honestly, I didn't write or approve the backgroundEvents code (before I got heavily involved). Personally believe that the onSelectEvent should never fire on background events, and personally would prefer the 'no-overlap' as the only layout for background events. But that's me, and I worry that attempting to change that now might upset some existing implementations. (Maybe those changes in the 'next' version...)

@chazzlabs
Copy link
Contributor Author

Thanks for the feedback! I'll go ahead and make the necessary updates to have backgroundEvents behave similarly to events with respect to their size and position. I agree that the other enhancements seem more appropriate for a future version given the potential breaking changes for some folks.

@chazzlabs
Copy link
Contributor Author

I'm not sure of the rationale behind why backgroundEvents were styled differently than events with regard to layout, but my latest commit makes the layout styling consistent for both kinds of events which seems like what we wanted to achieve.

@cutterbl
Copy link
Collaborator

@chazzlabs Looks great. Nice work.

@cutterbl cutterbl merged commit d2b7c23 into jquense:master Feb 21, 2024
1 check passed
Copy link

🎉 This PR is included in version 1.10.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants