no need to wrap with Fragment <>

Thanks for it, removed here.

I'm confused since it is still showing 😅 This comment also applies to AlertHeader above. Tested locally to check that this works without the fragments

Sorry I pasted the wrong link, I pushed this change to the PR where I was applying the changes requested by Sayalee.

rather than adding new props to pass child components to override existing components, I think it'd be better to abstract the logic so that shared logic resides in AlertModal or could be used by AlertModal. Shared logic could also be separate, reusable components.

The labels have the same values so we can remove confirmAlertModalAcknowledge and reuse the existing alertModalAcknowledge

The customAcknowledgeCheckbox is very similar to the existing one in AlertModal. We could move the existing AcknowledgeCheckbox outside of this component and into its own file to be reused. If all AlertModals are guaranteed to have these AcknowledgeCheckbox, we could consider not passing customAcknowledgeCheckbox, but more specific params

similar comments for the other "custom" child components

Great point, AcknowledgeCheckbox is a good candidate to abstract the logic.
I created an AcknowledgeCheckboxBase component to abstract the logic and re-utilized it into the confirm-alert-modal.tsx.
Unfortunately, some of those internal components changed over the most recent PRs such as #24214 and #24400. I will reassess the customAcknowledgeButton in the PRs mentioned to see if we can do something similar.

The storybook pages lock up the storybook left navigation (still seen but unusable), and the controls are unusable since there is no way to hide the modal. We should be able to utilize the component-library Modal features as expected. Similarly, to https://metamask.github.io/metamask-storybook/?path=/docs/components-componentlibrary-modal--docs

This behavior was introduced before this PR but would be nice to include.

Thanks for this suggestion. I've fixed MultipleAlertModal, ConfirmAlertModal and AlertModal stories.

Q: will we add UI support for "Security advice by Blockaid" in this PR or a future iteration?

There was some discussion on removing the Security advice by... So I ended up leaving to the last PR (implement alert to personal sign) the introduction of the provider in the banner and now into the modal here. My bad I should have described it here.

The current implementation has the close button above a divider line though the Figma version has no divider line and the UI appears closer to the close button

Figma

That is a good point I asked Sayalee because there are some inconsistencies in the figma and it makes sense to have a clear separation of the header and the content.
In any case, I have reduced the distance here already, and depending on Syalee's feedback I will push it to a subsequent PR.

rather than custom why not turn AlertModal into a presentational component so that it is not dependent on Alert? or... why does ConfirmAlertModal not utilize the selectedAlert system? Is it possible only multiple-alert-modal utilizes the "selectedAlert" logic?

e.g. something like, instead of the current MultipleAlertModal

    <AlertModal
      ownerId={ownerId}
      onAcknowledgeClick={handleAcknowledgeClick}
      alertKey={selectedAlert.key}
      onClose={onClose}
      headerStartAccessory={
        <PageNavigation
          alerts={alerts}
          onBackButtonClick={handleBackButtonClick}
          onNextButtonClick={handleNextButtonClick}
          selectedIndex={selectedIndex}
        />
      }
    />
  );

    <AlertModal
      ownerId={ownerId}
      onAcknowledgeClick={handleAcknowledgeClick}
      title={selectedAlert.reason}
      details={selectedAlert.alertDetails}
      message={selectedAlert.message}
      severity={selectedAlert.severity}
      onClose={onClose}
      headerStartAccessory={
        <PageNavigation
          alerts={alerts}
          onBackButtonClick={handleBackButtonClick}
          onNextButtonClick={handleNextButtonClick}
          selectedIndex={selectedIndex}
        />
      }
    />
  

Thank you for the suggestion!
Initially, I considered using AlertModal purely as a presentational component for alerts as you suggested. However, I chose not to go this route because the ConfirmAlerModal does not have any specific alert it only knows the ownerId and the alertKey to render the MultipleAlertModal so the user can review those alerts.

Additionally, restructuring to use AlertModal would still require passing custom components within ConfirmAlertModal, which seems to diminish the benefit of making AlertModal purely presentational.

Okay I see now. ConfirmAlertModal also renders MultipleAlertModal which I missed the first time. I also see that it also passes the selected alert related props ownerId and alertKey to AlertModal. Thanks for the clarification!

[suggestion] could rename customAlertTitle to customTitle as "Alert" is redundant here

Renamed customAlertTitle to alertTitle. c68451f

Okay I see now. ConfirmAlertModal also renders MultipleAlertModal which I missed the first time. I also see that it also passes the selected alert related props ownerId and alertKey to AlertModal. Thanks for the clarification!

[suggestion] can rename customAlertDetails → customDetails

Renamed customAlertDetals to customDetails c68451f .

I'm confused since it is still showing 😅 This comment also applies to AlertHeader above. Tested locally to check that this works without the fragments

what if we renamed ConfirmAlertModal to ConfirmMultipleAlertModal? The thought behind this is that is does also handle multiple alerts so this would provide more context

onFinalAcknowledgeClick is handled as the regular onClose. It does seem redundant that the user will acknowledge each alert individually and then need to acknowledge the final checkbox again after. maybe we could consider checking the final box if all others are checked cc: @SayaGT

current behavior:
https://github.com/MetaMask/metamask-extension/assets/20778143/da7b3915-492a-4fd7-9c5d-981ef805ba9b

It is happening like this because in the storybook it shows the full behaviour of the component, but if the user already acknowledges the alert using the Alert row component it would display only the ConfirmAlertModal component.

Removed comment c68451f.

Applied changes here.