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

Multiple components are potentially broken when removing from the DOM and adding them back #451

Closed
ex37 opened this issue Jun 2, 2021 · 1 comment
Assignees
Labels
bug Things that aren't working right in the library.

Comments

@ex37
Copy link

ex37 commented Jun 2, 2021

Hi @claviska, am duplicating my follow up message for #443 into a separate issue, cause looks like you missed it.

I spent some time and reviewed other occurrences where firstUpdated is used, and just by looking at the code (haven't tried to reproduce any of these), here is what I found:

https://github.com/shoelace-style/shoelace/blob/next/src/components/icon-button/icon-button.ts#L41

focusVisible.observe(this.button); should be moved to connectedCallback

https://github.com/shoelace-style/shoelace/blob/next/src/components/rating/rating.ts#L66
https://github.com/shoelace-style/shoelace/blob/next/src/components/details/details.ts#L68

exactly same issue

https://github.com/shoelace-style/shoelace/blob/next/src/components/tooltip/tooltip.ts#L103

this.popover = new Popover(this.target, this.positioner); should be moved to connectedCallback,
as well as all of the event listeners. Maybe also this.target = this.getTarget();, not sure about this one, but overall in the current state I find tooltip component not enought dynamic and ended up wraping it into my own one. If you are interested I can collaborate my thoughts on this component.

https://github.com/shoelace-style/shoelace/blob/next/src/components/textarea/textarea.ts#L141
this.resizeObserver.observe(this.input); should be moved

https://github.com/shoelace-style/shoelace/blob/next/src/components/tab-group/tab-group.ts#L65
this.resizeObserver.observe(this.nav); and the creation of this.mutationObserver should be moved

@ex37 ex37 added the bug Things that aren't working right in the library. label Jun 2, 2021
@claviska
Copy link
Member

claviska commented Jun 2, 2021

Thanks for taking the time to go through these. A lot of this is probably left over from the original Stencil conversion. I've addressed all of the issues you've mentioned above and reworked a few others in b0921b5.

am duplicating my follow up message for #443 into a separate issue, cause looks like you missed it

This was fixed for dropdown in d7bf0bd.

this.popover = new Popover(this.target, this.positioner); should be moved to connectedCallback

The popover utility was removed in d720121, so that was already fixed.

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

2 participants