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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor confirm modal #3876

Merged
merged 6 commits into from May 12, 2023
Merged

Refactor confirm modal #3876

merged 6 commits into from May 12, 2023

Conversation

eikhr
Copy link
Member

@eikhr eikhr commented May 7, 2023

Description

Replaces the ConfirmModalWithParent-component which used a lot of confusing higher-order-component trickery with a simpler functional component. The refactored component uses the Function as Child Component pattern to pass a openConfirmModal-function to the children, allowing f.ex. a button to open the modal. This makes for way more readable code imo.

Also rewrote the modal logic from class-component to function with hooks.

Aaand I also did the same thing for AttendanceModal

Result

No visual or functional changes, just cleaner code 馃Ъ

Testing

  • I have thoroughly tested my changes.

I have tested multiple confirm modals to ensure no changes in behaviour. I have also double-checked that I set the onClick correctly in all the usages of the new component.


Resolves ... (either GitHub issue or Linear task)

@eikhr eikhr added review-needed Pull requests that need review technical-debt Pull requests that reduces technical debt chore Pull requests that does something "boring", yet important, e.g. cleaning up code labels May 7, 2023
Copy link
Member

@LudvigHz LudvigHz left a comment

Choose a reason for hiding this comment

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

LGTM

app/components/Modal/ConfirmModal.tsx Show resolved Hide resolved
@eikhr eikhr requested a review from LudvigHz May 7, 2023 20:02
@eikhr eikhr force-pushed the refactor-modal-dropdown branch from 5676480 to 2deddae Compare May 7, 2023 21:27
Copy link
Member

@LudvigHz LudvigHz left a comment

Choose a reason for hiding this comment

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

Cool. Some missed low-hanging fruits in the refactoring of the AttendanceModal but overall super clean. This pattern is a lot better than the old HOCs 馃挴

app/components/UserAttendance/AttendanceModalContent.tsx Outdated Show resolved Hide resolved
app/components/UserAttendance/AttendanceModalContent.tsx Outdated Show resolved Hide resolved
app/components/UserAttendance/AttendanceModalContent.tsx Outdated Show resolved Hide resolved
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Looks better! 馃挴
Our modals have been a mess for a long time, so this was much needed!!

app/components/Modal/ConfirmModal.tsx Outdated Show resolved Hide resolved
app/components/UserAttendance/AttendanceModalContent.tsx Outdated Show resolved Hide resolved
@ivarnakken ivarnakken added types Pull requests that improve or fix types bug-fix Pull requests that fix a bug labels May 8, 2023
@eikhr eikhr force-pushed the refactor-modal-dropdown branch 2 times, most recently from 173bcc7 to 2c9b7f9 Compare May 11, 2023 10:56
@eikhr eikhr enabled auto-merge May 12, 2023 14:47
@eikhr eikhr merged commit d59e7dd into master May 12, 2023
3 of 4 checks passed
@eikhr eikhr deleted the refactor-modal-dropdown branch May 12, 2023 14:51
@ivarnakken ivarnakken mentioned this pull request Jun 8, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Pull requests that fix a bug chore Pull requests that does something "boring", yet important, e.g. cleaning up code review-needed Pull requests that need review technical-debt Pull requests that reduces technical debt types Pull requests that improve or fix types
Projects
None yet
3 participants