fix: Enhance confirmation modal provider to offer customizable option…

[igarashitm]

…s/buttons

Fixes: #1537

[sonarcloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[codecov]

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (main@f95f50e). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...c/providers/action-confirmation-modal.provider.tsx	96.42%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1545   +/-   ##
=======================================
  Coverage        ?   79.96%           
  Complexity      ?      259           
=======================================
  Files           ?      276           
  Lines           ?     7787           
  Branches        ?     1531           
=======================================
  Hits            ?     6227           
  Misses          ?     1500           
  Partials        ?       60           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lordrip]

TL;DR
Hi @igarashitm,

I think we can simplify the modal in the light of the intended changes, but I reckon this is a building block for another feature, so I want to propose a couple of alternatives:

1. Move forward with this PR as is and then we will revisit it at a later stage
2. We introduce the changes in this PR or in another one

Either option is good 👍 , if you wanna know the reasoning behind, please check the paragraph below.

---

Hi @igarashitm,

I would like to comment a couple of topics:

1. Having numeric indexes for the key attribute for React is not ideal, maybe in this particular case won't matter much as the array is static. Nevertheless, what you receive as output (1, 2, 0) perhaps is not as convenient, especially because it looks like despite changing the return type of the modal, is still working for the existing DeleteStep and DeleteGroup? 😄
2. Since this modal is used beyond simply deleting items, I think we could simplify the logic by removing everything that is Delete-related and assume the consumer will mention what exactly needs to be shown at all times.
   Things like:

      options.buttonOptions
        ? setButtonOptions(options.buttonOptions)
        : setButtonOptions([{ index: ACTION_INDEX_CONFIRM, buttonText: 'Confirm', variant: ButtonVariant.danger }]);

can be simplified to:

      setButtonOptions(options.buttonOptions)

I understand this is a building block for another feature, so feel free to move forward as-is and we can revisit it later, alternatively, I can lend a hand if you want as well.

Thanks for handling this 👍

[igarashitm]

Hi @lordrip thanks for the comments 👍

I'd start from this particular question

it looks like despite changing the return type of the modal, is still working for the existing DeleteStep and DeleteGroup

Yes and I kinda tried to - fix the cancel index to 0 which is evaluated to false, then we could keep the existing code using the modal as-is. This also affects why I pass the default Confirm button option when options.buttonOptions is not passed in, so that to be convenient for the existing modal user and also future users that just want Confirm and Cancel. But if you think it's fine to make it generic and let all modal users pass buttons, I can make that change.

[igarashitm]

We decided to go with this for now, a clean up would come later.