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

Default values not used when attributes are removed #281

Closed
dvic opened this issue Dec 9, 2020 · 4 comments
Closed

Default values not used when attributes are removed #281

dvic opened this issue Dec 9, 2020 · 4 comments
Assignees
Labels
bug Things that aren't working right in the library.

Comments

@dvic
Copy link

dvic commented Dec 9, 2020

Describe the bug
The sl-button component (but probably also other components) do not properly use default values when attributes are removed from the element after it has been initialized.

To Reproduce
Render a button:

<sl-button>Click me</sl-button>

The DOM will be:

<sl-button type="default" size="medium">Click me</sl-button>

It will look like:
image

Remove from the the DOM the type attribute, the button looks like:
image

Remove from the the DOM the size attribute, the button looks like:
image

Expected behavior
When removing an attribute (like type) I expected the component to render in the same way as the default value.

Screenshots
See above.

Desktop (please complete the following information):

  • OS: Mac OS
  • Browser: Firefox
  • Version: 83

Additional context
I encountered this problem when using Phoenix LiveView (https://www.phoenixframework.org/). The diffing algorithm removes the reflected attributes (type and size) upon each render and causes the components to render incorrectly. When I remove reflect: true everywhere, the problem goes away. But I'm not sure if this is possible or a breaking change.

@dvic dvic added the bug Things that aren't working right in the library. label Dec 9, 2020
@claviska
Copy link
Member

claviska commented Dec 9, 2020

Most components are designed such that props map to internal classes, and those classes dictate how the component is styled. When a component initializes, it sets default values for any missing props. If you remove those attributes/properties from the DOM, naturally, things bound to them (i.e. classes) will be affected.

When tinkering with the DOM, you can change an attribute or property however you like. But supplying invalid values (including null) may have ill consequences that the library shouldn't have to account for. The correct way to "unset" a prop is to set it to its default value, not remove it.

@dvic
Copy link
Author

dvic commented Dec 9, 2020

Thanks for the prompt response!

But supplying invalid values (including null) may have ill consequences that the library shouldn't have to account for.

I mostly agree with you but not with the fact that omitting an attribute is the same as providing an invalid value.

The correct way to "unset" a prop is to set it to its default value, not remove it.

Unfortunately this is not possible with a framework like LiveView or Livewire (the Laraval equivalent of LiveView) so it makes the use of this library not possible (afaik), which is a shame :)

By the way: I just checked if https://github.com/material-components/material-components-web-components suffers from the same issue but this seems not to be the case. When Liveview removes a reflected attribute in one of those components the components acts as if the default value was passed.

@claviska claviska changed the title default values not used when attributes are removed Default values not used when attributes are removed Dec 23, 2020
@claviska
Copy link
Member

This is an opinionated design decision that I'm not concerned about changing. Components come with default values, and removing an attribute will, as one would expect, cause the corresponding property to be removed. The amount of boilerplate this would require isn't worth the benefit.

The diffing algorithm removes the reflected attributes (type and size) upon each render and causes the components to render incorrectly.

This sounds like a problem with the framework, because things like <sl-alert open> will be impossible to render since open is also reflected and it won't display otherwise. If the framework truly is borking attributes, there are likely bigger problems than default values.

@petejodo
Copy link

Unfortunately this is not possible with a framework like LiveView or Livewire (the Laraval equivalent of LiveView) so it makes the use of this library not possible (afaik), which is a shame :)

This isn't necessarily true though, right? It just means that when using those frameworks, you should always be explicit and not depend on default values

If the framework truly is borking attributes, there are likely bigger problems than default values.

I don't think the frameworks are doing anything particularly wrong, they send html down the wire (that's a simplification of what's actually happening) and if something changes that html such that what the framework is tracking is no longer consistent, there's not really much they can do. That said, I don't think shoelace is doing anything wrong either, it's just an incompatibility between the two models.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library.
Projects
None yet
Development

No branches or pull requests

3 participants