Modal - Fix functionality for isDismissDisabled

[didoo]

📌 Summary

While working on the ChallengeModal component for HDS+ (https://github.com/hashicorp/hds-plus/pull/32) I started to test if it was possible to use the @isDismissDisabled argument to disable the dismiss of the modal while the content of the form was being validated (and potentially the consumer-side execution completed).

I checked in our HDS showcase, and I noticed that there were no specific demos for this functionality, so I started a new branch to add a scenario for this. While testing the use case, I noticed it didn't work, so I found that there was a bug, and I started to fix it.

I then looked at the Flyout component, which is very similar, and I noticed that this functionality was completely missing (despite exposing and documenting a @isDismissDisabled argument). We (well, I) missed this "detail" in this code review: https://github.com/hashicorp/design-system/pull/1887/files.
After conversation with @alex-ju I've removed the previous commits.

This PR aims to have the functionality work as expected in both components, and have it covered in the showcase and in the integration tests.

🛠️ Detailed description

In this PR I have:

• added demo example to the showcase for Modal to test isDismissDisabled functionality (used to TDD)
• removed jsdoc comments in Modal backing class (as agreed before)
• converted isDismissDisabled from tracked variable to getter in Modal backing class (this was the bug: essentially once the component was rendered the value of the tracked this.isDismissDisabled variable was not updated, even if the argument was updated, so I had to use a getter instead of a tracked variable)
• updated integration test of isDismissDisabled functionality for Modal

👀 Preview: https://hds-showcase-git-showcase-modal-flyout-isdismi-67f0a4-hashicorp.vercel.app/components/modal

📸 Screenshots

Before:

before.mov

After:

after.mov

---

👀 Component checklist

• Percy was checked for any visual regression
• A changelog entry was added via Changesets if needed (see templates here)

💬 Please consider using conventional comments when reviewing this PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
hds-showcase	✅ Ready (Inspect)	Visit Preview	Oct 8, 2024 5:14pm
hds-website	✅ Ready (Inspect)	Visit Preview	Oct 8, 2024 5:14pm

[alex-ju]

I then looked at the Flyout component, which is very similar, and I noticed that this functionality was completely missing (despite exposing and documenting a @isDismissDisabled argument). We (well, I) missed this "detail" in this code review: https://github.com/hashicorp/design-system/pull/1887/files.

As a background, the Flyout component was not meant to have an isDismissDisabled argument (was a conscious decision at the time) and, as such, it is not documented. We introduced it only for the Modal and refactored it in #1456. If we want to introduce it now for the Flyout, this change would need to be flagged as a feature.

[didoo]

As a background, the Flyout component was not meant to have an isDismissDisabled argument (was a conscious decision at the time) and, as such, it is not documented. We introduced it only for the Modal and refactored it in #1456. If we want to introduce it now for the Flyout, this change would need to be flagged as a feature.

@alex-ju my bad, I saw isDismissDisabled in the Args signature of the Flyout and I thought it was an expected feature. I'm going to remove the commits, and add a new one that removes that argument from the signature.

[didoo]

@alex-ju FYI I have opened a bug in Jira for a related issue I've found: https://hashicorp.atlassian.net/browse/HDS-3972
I've tried to investigate but with no luck, so I'll leave it to the Core squad to prioritize and investigate if/when possible.

[alex-ju]

Thanks for looking into this and recording the related issue in Jira! Great addition to the showcase as well.

[alex-ju]

👍

[didoo]

@alex-ju thanks for the prompt review. I'm going to merge it now.

One consideration: while technically this is just a patch, a bug fix, in practice it may lead to "breaking" changes in the consumers if their implementation in code was not correct. Essentially, since our implementation was not updating the value once set at initial render, the modal were actually dismissable even if the value of @isDismissDisabled was set to true (eg. on processing the modal). I don't expect real impact on the consumers, but one never knows, so for future reference here are the places where this argument is dynamically set

• https://github.com/hashicorp/cloud-ui/blob/988fc423a79107e6359b01aaddc6baf21bbc8a12/hcp/app/components/manage-mfa/enable/index.hbs#L4
• https://github.com/hashicorp/cloud-ui/blob/988fc423a79107e6359b01aaddc6baf21bbc8a12/engines/vagrant/addon/components/modal/index.hbs#L6
• https://github.com/hashicorp/cloud-ui/blob/988fc423a79107e6359b01aaddc6baf21bbc8a12/engines/packer/addon/components/registry/modal/deactivate.gts#L82
• https://github.com/hashicorp/cloud-ui/blob/988fc423a79107e6359b01aaddc6baf21bbc8a12/hcp/app/components/manage-mfa/disable/index.hbs#L4
• https://github.com/hashicorp/vault/blob/25ce9912347d80ad90242c0a9d6f2203b1d0f94c/ui/lib/replication/addon/templates/mode/secondaries/add.hbs#L59

I don't know if you want to do some tests before doing the actual HDS version upgrade in these consumers. Probably an overkill, but given our recent struggle with the HDS version update, something to consider.

[alex-ju]

I don't know if you want to do some tests before doing the actual HDS version upgrade in these consumers. Probably an overkill, but given our recent struggle with the HDS version update, something to consider.

I am planning to test in products before releasing, so thanks for pointing to the current instances for this change.