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

feat(form): add reset functionality #799

Merged
merged 5 commits into from
Jun 28, 2022

Conversation

alenaksu
Copy link
Collaborator

@alenaksu alenaksu commented Jun 23, 2022

This PR adds the form reset functionality to SL components.

Two new options has been added to FormSubmitController:

  • defaultValue - returns the control's default value
  • setValue - a function used to set the control's value

The idea is to set the value originally defined in the component attribute when the form's reset event is fired (spec). To reset the form through the SL button, this implementation uses the same approach used for the submission.

@vercel
Copy link

vercel bot commented Jun 23, 2022

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

Name Status Preview Updated
shoelace ✅ Ready (Inspect) Visit Preview Jun 28, 2022 at 7:14PM (UTC)

@claviska
Copy link
Member

Thanks for tackling this. I like your approach and I'll spend more time testing it out soon. One thing I noticed is it's missing some controller options on a few form controls. At a glance, I don't think they're required since they should use the Form Controller's default values — but I just want to confirm that they're all covered.

Here's a list of all controls that submit:

  • button
  • checkbox
  • color-picker
  • input
  • radio
  • radio-button
  • range
  • select
  • switch
  • textarea

@claviska
Copy link
Member

I had a chance to test each form control individually. All of the aforementioned controls reset correctly except for the following:

Checkboxes don't reset to their initial checked state. Uncheck the checkbox, then hit reset. The checkbox is still unchecked.

<form>
  <sl-checkbox checked>Click me</sl-checkbox>
  <br><br>
  <sl-button type="reset" variant="primary">Reset</sl-button>
</form>

Radios don't reset to their initial checked state. Select a different option, then hit reset. No radios are checked.

<form>
  <sl-radio-group label="Select an option">
    <sl-radio name="option" value="1" checked>Option 1</sl-radio>
    <sl-radio name="option" value="2">Option 2</sl-radio>
    <sl-radio name="option" value="3">Option 3</sl-radio>
  </sl-radio-group>
  <br /><br />
  <sl-button type="reset" variant="primary">Reset</sl-button>
</form>

Radio buttons don't reset to their initial checked state. Select a different option, then hit reset. No radio buttons are checked.

<form>
  <sl-radio-group label="Select an option">
    <sl-radio-button name="a" value="1" checked>Option 1</sl-radio-button>
    <sl-radio-button name="a" value="2">Option 2</sl-radio-button>
    <sl-radio-button name="a" value="3">Option 3</sl-radio-button>
  </sl-radio-group>
  <br /><br />
  <sl-button type="reset" variant="primary">Reset</sl-button>
</form>

Switches don't reset to their initial checked state. Uncheck the switch, then hit reset. The checkbox is still unchecked.

<form>
  <sl-switch checked>Switch</sl-switch>
  <br /><br />
  <sl-button type="reset" variant="primary">Reset</sl-button>
</form>

I also noticed that defaultValue isn't exposed for any form control, so it's not possible to change the default value later on. This isn't necessarily a blocker for this PR, but it's a nice to have.

Let me know what you think!

@alenaksu
Copy link
Collaborator Author

alenaksu commented Jun 24, 2022

Hi @claviska, yes I think that exposing defaultValue and defaultChecked would be a good addition.

A possible limitation I see here is that the value and checked properties are configured to reflect back the updates to their attributes, this means that also the default value will change. And I suppose that turning off the reflection would be a sort of breaking change, since some people, for example, may be using the attributes for styling purposes.
What do you think?

I'll take a look at the components that are not resetting properly, and I'll also investigate a bit further about this topic

@claviska
Copy link
Member

And I suppose that turning off the reflection would be a sort of breaking change, since some people, for example, may be using the attributes for styling purposes.

💯

Those should continue to reflect to remain consistent with the rest of the library. We’ll need to work around that, possibly with an extra property.

@alenaksu
Copy link
Collaborator Author

alenaksu commented Jun 27, 2022

I think I found a possible solution.

The spec says that the defaultValue/defaultChecked should reflect the value set in the attribute, so we have two option to update the default value:

  1. Update the attribute of the element
  2. Set the value using the property

For the second, we just have to update the property value and that's all.

For the first, we could listen the attributeChangedCallback callback, and check whether the new value of the attribute is equal to the value of the corresponding IDL attribute, for example checked. If the values are different, then it means that the attribute has been updated and we can update also the defaultValue, otherwise it means that the property has been update first and then reflected to the attribute, in this case we do nothing.

// pseudo code
class Element {
    attributeChangedCallback(name, oldValue, newValue) {
         if (name === 'checked') {
             if (this.checked !== newValue) 
                 this.defaultChecked = newValue;
         }
    }
}

The code above could be abstracted in a decorator for simplicity (that I'm going to push soon)

class Element {
    @property({ type: Boolean, reflect: true }) 
    checked = false;

    @defaultValue('checked')
    defaultChecked = false;
}

What do you think?

@alenaksu
Copy link
Collaborator Author

this is the idea implemented in the checkbox component
d7ce4ac

@alenaksu
Copy link
Collaborator Author

I added the defaultValue/defaultChecked properties to all the components that need it. This solved also the issues I had with some of them.

I implemented some unit tests as well.

@claviska
Copy link
Member

Great work on this. Thanks for taking the time to submit this, and especially for adding tests. There are a couple of sort order lint rules and a failing focus test in WebKit, but I'll handle those.

@claviska claviska merged commit b2cf3a5 into shoelace-style:next Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants