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

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

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

Conversation

HenriRabalais
Copy link
Collaborator

@HenriRabalais HenriRabalais commented Dec 16, 2024

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

@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

@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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants