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

Add exclusivity to prevent 12:00AM leaking into the next day event in the week #2622

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

Conversation

geraldhoxha95
Copy link

@geraldhoxha95 geraldhoxha95 commented Jul 13, 2024

Fixes #2617

By passing in a minute as unit, we can prevent an event that ends at exactly 12:00 AM from leaking into the next day, to more align with functionality from calendars like Outlook.

Screen.Recording.2024-07-13.at.8.54.36.AM-1.mov

@anjalikalsariya
Copy link

anjalikalsariya commented Jul 19, 2024

Hello folks,

Any one please review this PR. Actually I need this change in package for my current project.

Thanks

@cutterbl
Copy link
Collaborator

@geraldhoxha95 I need some example story and data (maybe in the Event Durations area of Storybook) where I can thoroughly test the assumption here. Code looks good, from what I can tell, but seeing is believing.

@geraldhoxha95
Copy link
Author

@cutterbl Give me a few days, I will try to do it over the weekend.

@geraldhoxha95
Copy link
Author

@cutterbl I added a few examples to the Event Durations.

@anjalininjadev
Copy link

Hi @cutterbl @geraldhoxha95

Is there any chance to merge and release this PR by the weekend? or Should wait for it for next week ?

Thanks

@geraldhoxha95
Copy link
Author

@anjalininjadev I do not have the ability to merge this request.

@anjalikalsariya
Copy link

@anjalininjadev I do not have the ability to merge this request.

Okay @geraldhoxha95 , np.

@cutterbl Can you please take a look and merge it ? when you get a moment. We need this fix because we are going release first version of our product soon.

@cutterbl
Copy link
Collaborator

Looking over this code and seeing the changes you made with the moment localizer. What about the other localizers (Luxon, dayjs, etc)?

@geraldhoxha95
Copy link
Author

I've been swamped this past month so haven't had a chance to get back to you. @anjalikalsariya @anjalininjadev if this issue is still relevant to you, or if @cutterbl if you think this a change worth making, I will try to do more testing sometime this week.

@cutterbl
Copy link
Collaborator

@geraldhoxha95 I still think it's worth exploring

@geraldhoxha95
Copy link
Author

I will add more testing over the weekend

@geraldhoxha95
Copy link
Author

@cutterbl I added tests for the other use cases.

@cutterbl
Copy link
Collaborator

cutterbl commented Oct 3, 2024

@geraldhoxha95 Getting really close here. Added a few more code review notes.

@geraldhoxha95
Copy link
Author

@cutterbl
Sorry, where can i find the code review notes?

@cutterbl
Copy link
Collaborator

cutterbl commented Nov 5, 2024

@geraldhoxha95 Go to the Files Changed view and you'll see my last note there

@geraldhoxha95
Copy link
Author

files_changed-2.mov

@cutterbl Am I missing something? I am not seeing anything on the Files Changed view

@cutterbl
Copy link
Collaborator

@geraldhoxha95 Realy? You should see a review on the dayjs.js localizer...

@anjalikalsariya
Copy link

Hi @cutterbl @geraldhoxha95

I think we are near to PR merge. My team are waiting for this fix release. I would like to request to you guys , Please try to merge this fix and new release as you can.

Thanks.
Anjali K

@cutterbl
Copy link
Collaborator

@geraldhoxha95 You should see something like the following screenshot. You'll also need to merge in latest changes from master and resolve conflicts as well.
Screenshot 2024-11-26 at 8 33 42 AM

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.

Week view, Event showing twice
4 participants