Skip to content

[javascript] Split form and non-form modals and support custom modal footers - #10695

Open
MaximeBICMTL wants to merge 4 commits into
aces:mainfrom
MaximeBICMTL:refactor-modals
Open

[javascript] Split form and non-form modals and support custom modal footers#10695
MaximeBICMTL wants to merge 4 commits into
aces:mainfrom
MaximeBICMTL:refactor-modals

Conversation

@MaximeBICMTL

@MaximeBICMTL MaximeBICMTL commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Description

This PR refactors the LORIS modal to make two major changes:

  • Separate form and non-form general modals into separate reusable components.
  • Make modal footers optional and allow consumers to provide their own footer for the generic modal.

Architecture

  • The Modal component from jsx/Modals.tsx is split into three components:
    • Modal: The general non-form modal, which allows the consumer to provide the body and optionally the footer.
    • FormModal: The form modal, which is similar to the current modal and provides form and submit logic/controls.
    • ModalFrame: common architecture between the two modals, technically FormModal could also be made to depend directly upon Modal but this would require some higher-order component machinery because the form body and content should both be wrapper in a form element, and this wrapper is undesirable in the general case.
  • The styling relies less on bootstrap and more on flexbox for layout.
  • The modals can be scrollable, not sure if that was the case before or not.

Examples

Showcase of existing modal behavior: (added artificial delay to showcase animations)

2026-06-29.18-08-33.mp4

Showcase of new non-form modals with custom footers:

2026-06-29.18-13-40.mp4

@github-actions github-actions Bot added Language: Javascript PR or issue that update Javascript code Module: instrument_manager PR or issue related to instrument_manager module Module: data_release PR or issue related to data_release module Module: dataquery PR or issue related to (new) dataquery module labels Jun 26, 2026
@MaximeBICMTL
MaximeBICMTL marked this pull request as draft June 26, 2026 11:29
@MaximeBICMTL MaximeBICMTL added Category: Feature PR or issue that aims to introduce a new feature Category: Refactor PR or issue that aims to improve the existing code labels Jun 26, 2026
@MaximeBICMTL MaximeBICMTL changed the title [JS] Split form and non-form modal and support custom modal footers [JS] Split form and non-form modals and support custom modal footers Jun 26, 2026
@MaximeBICMTL
MaximeBICMTL marked this pull request as ready for review June 29, 2026 10:21
@MaximeBICMTL MaximeBICMTL changed the title [JS] Split form and non-form modals and support custom modal footers [javascript] Split form and non-form modals and support custom modal footers Jul 1, 2026
@HenriRabalais
HenriRabalais self-requested a review July 23, 2026 15:30
@MaximeBICMTL MaximeBICMTL added this to the 30.0.0 milestone Jul 24, 2026
@MaximeBICMTL
MaximeBICMTL changed the base branch from main to 29.0-release July 24, 2026 11:52
Comment thread jsx/Modal.tsx Outdated
}>;

export type FormModalProps = Omit<ModalProps, 'footer'> & {
onSubmit?: () => Promise<any> | any;

@HenriRabalais HenriRabalais Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

By splitting it into a specific FormModal I think we should require an onSubmit function to be passed. I can't think of a case where we would want a form with no submit function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hhhhm, I initially kept FormModalProps the exact same to not break backwards compatibility, but I just checked and it does appear that all call sites provide an onSubmit prop, so I think your call is right.

I pushed a new commit that makes onSubmit mandatory and also improves the typing of onSubmit and onSuccess by using a generic type rather than any.

Comment thread jsx/Modal.tsx
onSuccess?: (data: any) => void;
title?: ReactNode;
width?: string;
footer?: ReactNode;

@HenriRabalais HenriRabalais Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm a little hesitant to accept any ReactNode as a footer, but I do see the need for flexibility based on your examples... can you think of something more structured that we could implement? For example to make sure buttons stay bottom right, things are properly spaced, etc.

Now that I look at it, the same would apply for title. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hhhhm, I think I disagree here. Even though most modals would use buttons as the footer, that may not always be the case, and I actually have one such footer in my examples. I think using ReactNode is fine, no need to be too restrictive for UI IMO.

@github-actions github-actions Bot added Language: SQL PR or issue that update SQL code Language: PHP PR or issue that update PHP code Module: candidate_parameters PR or issue related to candidate_parameters module Module: document_repository PR or issue related to documen_repository module Module: redcap PR or issue related to redcap module labels Aug 11, 2026
@MaximeBICMTL
MaximeBICMTL changed the base branch from 29.0-release to main August 11, 2026 14:39
@MaximeBICMTL MaximeBICMTL removed Language: SQL PR or issue that update SQL code Language: PHP PR or issue that update PHP code labels Aug 11, 2026
@MaximeBICMTL MaximeBICMTL removed Module: candidate_parameters PR or issue related to candidate_parameters module Module: document_repository PR or issue related to documen_repository module Module: redcap PR or issue related to redcap module labels Aug 11, 2026
@MaximeBICMTL

Copy link
Copy Markdown
Contributor Author

@HenriRabalais I addressed your suggestions. Do you want to review this PR when you have time or should we assign it to someone else?

@HenriRabalais HenriRabalais left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for making onSubmit mandatory! Going back through it though I think we've missed a case. In batchEditForm.js the onSubmit is still passed conditionally:

onSubmit={Object.keys(list).length > 1 ? handleSubmit : undefined}

The list starts empty and the user scans specimens into it, so this is really "only let them submit once there are at least 2". The old Modal handled that by hiding the Save button when onSubmit was falsy. Now that FormModal always renders the button and requires onSubmit, when the modal opens with fewer than 2 specimens the Save button shows right away and clicking it does nothing (it ends up calling undefined(), which gets swallowed by the catch).

So I think there's a genuine use case here for gating submission, unless we decide we never want to prevent it. As I see it the options are:

  1. make onSubmit conditional again, but if it's not passed then FormModal disables the Save button instead of hiding it
  2. add a disabled boolean prop that toggles the Save button
  3. add a string prop for a little message/tooltip explaining why it can't be submitted yet (shown on hover, or at the bottom of the footer by default), where an empty/null value means it's submittable
  4. or just keep the Save button always enabled and never prevent submission

I honestly don't have a strong preference between them, they all work for me. If I was to design it, I would probably go with 3 with the message displayed in the footer. What do you think?

@MaximeBICMTL

MaximeBICMTL commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

@HenriRabalais Oh good find. I checked that all modals supplied an onSubmit attribute and that type checking passed, but I guess I should have looked more closely that undefined wasn't passed explicitly.

I implemented option 2 in my last commit, I prefer that over option 3 because I am not sure what should be the ideal validation UI (a tooltip on the button? a more complete error on the appropriate fields?). IMO a more complete reactive validation UI should be left to another PR.

I will note that this would have been caught by type checking if the code was typed, we need more TypeScript in LORIS!

@HenriRabalais HenriRabalais left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great, thanks for the quick turnaround! 🙂

@driusan driusan self-assigned this Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: Feature PR or issue that aims to introduce a new feature Category: Refactor PR or issue that aims to improve the existing code 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 Module: instrument_manager PR or issue related to instrument_manager module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants