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

Modals - function mismatch #6000

Closed
marceltrautmann opened this issue Apr 29, 2020 · 7 comments · Fixed by #6793
Closed

Modals - function mismatch #6000

marceltrautmann opened this issue Apr 29, 2020 · 7 comments · Fixed by #6793

Comments

@marceltrautmann
Copy link

https://www.carbondesignsystem.com/components/modal/code

The example given is a transactional modal (evident by the two buttons), yet – per the guidance on the usage page – transaction modals should not be dismissable by clicking outside the modal. In the example given it is dismissable just like a passive modal.

Given this is a reference used by many I suggest making the example follow the same guidance to help avoid the same confusion my team just did.

image
image

@jnm2377
Copy link
Contributor

jnm2377 commented May 1, 2020

Hi @marceltrautmann thanks for opening this issue. You bring up a good point. The functionality you see in our docs is coming directly from our Carbon package since ModalWrapper handles all of that already, so I'm going to transfer this issue to the /carbon monorepo since the changes need to be made there.

@jnm2377 jnm2377 transferred this issue from carbon-design-system/carbon-website May 1, 2020
@jnm2377
Copy link
Contributor

jnm2377 commented May 1, 2020

Note for devs: the component demo is using ModalWrapper, which handles the open/close. It seems like there should be a check to see if the modal is passive or not, and only be dismissible when you click outside if it's passive.

@asudoh
Copy link
Contributor

asudoh commented May 2, 2020

As of today, our click-outside logic works regardless of passive/transactional mode. CC @carbon-design-system/design to see if it should be changed.

Another note is that <ModalWrapper> is a demonstration-only code (the team of very early Carbon days somehow shipped it for an unknown reason and we haven't had a chance to remove it). https://carbon-website-git-fork-asudoh-modal-code.carbon-design-system.now.sh/components/modal/code#openingclosing-modal explains the alternate approach.

@aagonzales
Copy link
Member

I'm not sure how a <ModalWrapper> is different from the normal modal but the behaviors should be the same. A transactional modal should NOT be dismissible by clicking outside of the modal area.

@aagonzales
Copy link
Member

It looks like the <ComposedModal> also has the same problem.

@asudoh
Copy link
Contributor

asudoh commented May 4, 2020

Thank you for clarifying @aagonzales!

Wrt where <ModalWrapper> stands, the short answer is <ModalWrapper> is <Modal> and its launcher button combined. Why <ModalWrapper> exists is a technical topic; <Modal> and <ComposedModal> requires the launcher button implemented in application code, because not all modals come with a launcher button and providing it out-of-the-box may cause some leaky abstractions. I don't know the glory detail of <ModalWrapper> story, but it appears that the Carbon website needed a modal component with a launcher button for demonstrating modal, and they somehow decided to ship it from Carbon instead of making it website-only code.

@janchild
Copy link
Contributor

janchild commented Sep 1, 2020

@alisonjoseph Can we pls cue this up for the next sprint planning? This is a priority issue for the Watson Assistant team. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants