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

feat: past and overlapping ooo #17373

Merged
merged 8 commits into from
Nov 24, 2024

Conversation

Amit91848
Copy link
Contributor

@Amit91848 Amit91848 commented Oct 28, 2024

What does this PR do?

Screencast.from.2024-10-28.21-26-38.webm

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

Copy link

vercel bot commented Oct 28, 2024

@Amit91848 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added 💎 Bounty A bounty on Algora.io 🧹 Improvements Improvements to existing features. Mostly UX/UI labels Oct 28, 2024
@dosubot dosubot bot added the ✨ feature New feature or request label Oct 28, 2024
@dosubot dosubot bot added this to the Community Only milestone Oct 28, 2024
@graphite-app graphite-app bot requested a review from a team October 28, 2024 16:01
@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Oct 28, 2024
Copy link

graphite-app bot commented Oct 28, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (10/28/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (10/28/24)

1 label was added to this PR based on Keith Williams's automation.

@Praashh Praashh self-assigned this Oct 28, 2024

function Calendar({
className,
classNames,
fromDate,
toDate,
showOutsideDays = true,
disablePastDates = true,
Copy link
Contributor

@emrysal emrysal Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer this to be a minDate option (set by default to new Date()) - and nullable to allow all dates enabled.

Edit: This can be implemented to a minor tweak to the fromDate instead of introducing a new param.

Copy link
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@Praashh Praashh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes looks correct to me, tested in local as well. Thank you @Amit91848

Copy link
Contributor

github-actions bot commented Nov 1, 2024

E2E results are ready!

Copy link
Contributor

@anikdhabal anikdhabal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls add test to verify this

@Amit91848
Copy link
Contributor Author

Recording.2024-11-18.132444.mp4

Is something up with ooo page? I cannot load it and it straight up crashes the server without any error message. I have the latest pull from upstream

@Praashh
Copy link
Contributor

Praashh commented Nov 20, 2024

Recording.2024-11-18.132444.mp4
Is something up with ooo page? I cannot load it and it straight up crashes the server without any error message. I have the latest pull from upstream

nope, working for me

Copy link
Contributor

@Praashh Praashh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, can you please add a test case for overlapping OOOs?

@Amit91848
Copy link
Contributor Author

Added tests, sorry for the delay.

Pretty sure there is a memory leak in ooo page. Also maybe in other sub pages under /my-account

Screenshot 2024-11-20 205438

Screenshot 2024-11-20 205634

@Praashh
Copy link
Contributor

Praashh commented Nov 20, 2024

I'll check that

Copy link
Contributor

@Praashh Praashh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I notice that I don't think we need duplicate OOO, we only need overlapping OOOs.

@Amit91848
Copy link
Contributor Author

image

I notice that I don't think we need duplicate OOO, we only need overlapping OOOs.

Nice catch, made changes and updated tests

Copy link
Contributor

@Praashh Praashh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

@github-actions github-actions bot added Medium priority Created by Linear-GitHub Sync 🐛 bug Something isn't working labels Nov 24, 2024
@PeerRich PeerRich enabled auto-merge (squash) November 24, 2024 18:37
@PeerRich PeerRich merged commit 5af10a2 into calcom:main Nov 24, 2024
33 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 Bounty claim 💎 Bounty A bounty on Algora.io 🐛 bug Something isn't working community Created by Linear-GitHub Sync ✨ feature New feature or request 🧹 Improvements Improvements to existing features. Mostly UX/UI Medium priority Created by Linear-GitHub Sync ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-4769] currently cant have overlapping OOO [CAL-4631] allow OOO of the past and overlapping
5 participants