Skip to content

[JSX] Refactor Modal Component to Support Conditional Form Wrapping #9516

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.

Sign up for GitHub

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

Jump to bottom
Merged
driusan merged 1 commit into from
May 9, 2025
Merged

[JSX] Refactor Modal Component to Support Conditional Form Wrapping #9516

driusan merged 1 commit into from
May 9, 2025

Conversation

@HenriRabalais
Copy link
Collaborator

@HenriRabalais HenriRabalais commented Dec 16, 2024 •
edited
Loading

PR Description

This pull request refactors the Modal component to improve flexibility and maintainability by conditionally wrapping content in a FormElement if an onSubmit callback is provided.

Key Changes:

  • Conditional Form Wrapping:

    • Added logic to wrap content in a FormElement when onSubmit is passed as a prop.
    • Ensures consistent behavior for submitting forms or displaying static content.

  • Simplified JSX Structure:

    • Improved formatting and readability by restructuring the ternary operator.
    • Ensured consistent indentation and reduced code duplication.

  • Preserved Functionality:

    • Maintains existing behavior while improving code clarity and maintainability.

Purpose:

  • Enables better handling of forms inside the modal.
  • Prepares the component for future enhancements.

@HenriRabalais HenriRabalais added Beginner Friendly PR or Issue appears to be easy for someone to use to familiarize themselves with LORIS Language: Javascript PR or issue that update Javascript code Difficulty: Simple PR or issue that should be easy to implement, review, or test labels Dec 16, 2024
@driusan
Copy link
Collaborator

driusan commented Dec 17, 2024

@ridz1208 I think you're the one that wanted this, can you review?

@HenriRabalais
Copy link
Collaborator Author

HenriRabalais commented Dec 18, 2024

@driusan I've updated the uses of modal where nesting of a <form> tag was occurring — do you want to just take a look at those? And @ridz1208 could you look at the jsx/Modal.js file?

@HenriRabalais
Copy link
Collaborator Author

HenriRabalais commented Jan 9, 2025

@driusan if you have time, do you mind just looking at the dataquery changes while I wait on a review from the rest of it from @ridz1208 ?

@driusan
Copy link
Collaborator

driusan commented Jan 9, 2025

@HenriRabalais I am confused because I thought the <form> elements were being removed due to being duplicated elsewhere in the hierarchy but I don't see in this PR where that is. It doesn't seem to be in Modal.

@driusan
Copy link
Collaborator

driusan commented Jan 9, 2025

Nevermind, I just realized <FormElement> is a <form> element. I'll have to test this.

@driusan
Copy link
Collaborator

driusan commented Jan 9, 2025

The dataquery changes look minimal when I look at them with ?w=1 and seem to look fine on the frontend to me.

@HenriRabalais
Copy link
Collaborator Author

HenriRabalais commented Jan 13, 2025

@maximemulder would you mind reviewing this when you have some spare time? I don't think @ridz1208 has time at the moment.

@ridz1208
add form element to modal
908c956 
added changes from lack of rebase

updated formelement props

modify uses of modal to avoid form nesting

resolving lint
@HenriRabalais HenriRabalais force-pushed the 2024-12-16_addformtomodal branch from 0151ad4 to 908c956 Compare May 6, 2025 17:40
@github-actions github-actions bot added Module: data_release PR or issue related to data_release module Module: dataquery PR or issue related to (new) dataquery module labels May 6, 2025
@HenriRabalais
Copy link
Collaborator Author

HenriRabalais commented May 8, 2025

@driusan @ridz1208 this should be good to go now!

@driusan driusan merged commit f873f6e into aces:main May 9, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ridz1208 ridz1208 ridz1208 approved these changes

@maximemulder maximemulder Awaiting requested review from maximemulder

Assignees

@ridz1208 ridz1208

Labels

Beginner Friendly PR or Issue appears to be easy for someone to use to familiarize themselves with LORIS Difficulty: Simple PR or issue that should be easy to implement, review, or test Language: Javascript PR or issue that update Javascript code Module: data_release PR or issue related to data_release module Module: dataquery PR or issue related to (new) dataquery module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@HenriRabalais @driusan @ridz1208