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(ui): duplicate schedule warning #295

Merged

Conversation

adityamkk
Copy link
Contributor

@adityamkk adityamkk commented Oct 14, 2024

Issue: it is possible to bypass the schedule creation limit by selecting certain options.

Solution: I created a useEnforceScheduleLimit hook in the DialogProvider directory

These hook attempts to add/duplicate a schedule, but displays a warning dialog whenever the schedule limit is breached.
This hook is used in the CalendarSchedules, ScheduleListItem, and PopupMain components.

image


This change is Reviewable

@doprz doprz self-requested a review October 14, 2024 03:48
@IsaDavRod
Copy link
Member

Hey Aditya, please see the task in the Bug Tracker project and let me know if this explains more about the issue and how it can be fixed. Not sure if that is what your method handles.

Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

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

LGTM (still need review from Sam, Diego, or Elie) but the loophole seems to be fixed with this PR

Copy link
Collaborator

@Samathingamajig Samathingamajig left a comment

Choose a reason for hiding this comment

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

looks good in general, but there's a big chunk of duplicate code that should be reduced (see other comments for more details)

src/views/components/PopupMain.tsx Outdated Show resolved Hide resolved
src/views/components/common/ScheduleListItem.tsx Outdated Show resolved Hide resolved
@Razboy20
Copy link
Member

Ideally, we should have a standard component for this particular box so we can use it both when adding a schedule normally and when duplicating a schedule.

This is the perfect PR for that! 😄

Copy link
Collaborator

@Samathingamajig Samathingamajig left a comment

Choose a reason for hiding this comment

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

looks pretty good, couple of small changes requested to ensure code quality

code works on my machine 👍

src/views/components/PopupMain.tsx Outdated Show resolved Hide resolved
src/views/components/calendar/CalendarSchedules.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@Samathingamajig Samathingamajig left a comment

Choose a reason for hiding this comment

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

LGTM, awaiting re-review of UI from @IsaDavRod

Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

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

erm, looks giga chad to me

@Samathingamajig Samathingamajig merged commit 7346720 into Longhorn-Developers:main Oct 23, 2024
6 checks passed
@adityamkk adityamkk deleted the fix/duplicate-schedule branch October 23, 2024 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants