carbon-design-system / carbon-components-svelte

Svelte implementation of the Carbon Design System
https://svelte.carbondesignsystem.com
Apache License 2.0
2.71k stars 261 forks source link

Modal with Form on:submit method #310

Open albertms10 opened 4 years ago

albertms10 commented 4 years ago

When a Form is placed inside a Modal, the Modal on:submit method is called with a target: null property either clicking on the primary button or hitting enter (as shouldSubmitOnEnter is true by default). Shouldn’t it contain a reference to the HTMLFormElement instead? Or do I need to manually manage on:click primary button to submit the form?

<Modal
  ...
  hasForm
  on:submit={(e) => {
    console.log('modal', e);
  }}
>
  <Form
    on:submit={(e) => {
      console.log('form', e); // Is never printed
    }}
  >
    <TextInput name="name" labelText="Name" required />
    <TextInput name="email" labelText="Email" required />
  </Form>
</Modal>
modal CustomEvent {..., target: null}
metonym commented 4 years ago

This is most definitely a bug. I'd expect <Modal on:submit /> to return a reference to the form. Would you like to work on this?

albertms10 commented 4 years ago

I don’t know if I understand what should be going on here, but, for the Modal on:submit event to contain a reference to the Form as a target, it should be placed inside the Modal component definition and not as a child of it, right?

So, @metonym, what is the expected way of this to work in an automatic way?—without having to manually manage the Form reference to the Modal outside of it.

metonym commented 4 years ago

After looking at this again, I don't think the Form state should be managed by Modal. That being said, I feel that a prop like hasForm ideally should be set automatically if a Form is present. hasForm is purely cosmetic.

albertms10 commented 4 years ago

As Svelte cannot know which elements are contained in the component’s slot, hasForm should be there to set the appropriate style—or the CSS styles should take that into account. So, as you stated, the Modal on:submit should not be considered to also submit the Form content. To make things easy, something like what is described in this REPL—by using stores—should make it work. If this is the expected behaviour, we should add an example to the docs, as well.

brunnerh commented 2 years ago

Both hasForm and hasScrollingContent seem lacking in terms of documentation.

sallaben commented 2 years ago

shouldSubmitOnEnter doesn't seem to work at all! Documentation states it will trigger the primary button click event, but it does not.

metonym commented 2 years ago

@sallaben Good catch. Tracking in a separate issue: https://github.com/carbon-design-system/carbon-components-svelte/issues/1005

WilliamDiakite commented 2 years ago

The solution I've used so far is to pass the id of the form element to the Modal. Because the button set of the modal is outside the form, clicking on the primary button wont trigger a sumbit on the form. Adding type="submit" and a form attribute to the primary button with value of formId does the trick.

Here is a REPL illustrating the suggested solution.

I would be happy to make a pull request to fix this bug asap.

metonym commented 2 years ago

@WilliamDiakite A PR would be appreciated