m- icon indicating copy to clipboard operation
m- copied to clipboard

Question: M-Alert

Open eavichay opened this issue 3 years ago • 4 comments

M-Alert takes all children, creates a div and places everything inside. Seems like a proper usage of <slot> could solve it.

It also breaks interoperability with other libraries by mutating the DOM. Example:

Lib X has a component that uses m-alert. Render phase complete, then m-alert alters the DOM. Lib X Might break.

eavichay avatar Oct 16 '20 19:10 eavichay

Ah yes, good questions.

I avoided slot because so far there has been no need for Shadow DOM and I'm trying to avoid Shadow DOM for a number of reasons.

I am also finding mutating DOM owned by other libs is not a problem. There's probably edge cases, but so far so good and I just don't want jump into Shadow DOM until I know it (and the challenges/constraints that come with it) are absolutely necessary. I do know of one challenge with Svelte, but that will need a little mutation observer, not Shadow DOM.

jfbrennan avatar Oct 19 '20 06:10 jfbrennan

I Disagree. Moving the children breaks. That is exactly the point of having a <slot>, and you can add css rules within your component.

eavichay avatar Oct 19 '20 09:10 eavichay

Can you give some more details about how it's breaking? I'd like to look into it, I just need more info.

jfbrennan avatar Oct 21 '20 04:10 jfbrennan

Sure. React implements virtual DOM. (Just an example of one of many).

If I place a dynamic content inside m-alert, when React attempts to replace the content with another, it can break (depends on the DOM replacement).

An example of how to break it, it by replacing the content of an m-alert from span to div. Since it's not a text-node change, React will fail to remove the child from the known parent, as it is no longer the parent.

Click the button, and the whole app crashes.

Example: Demo: https://react-7yrppz.stackblitz.io Code: https://stackblitz.com/edit/react-7yrppz?file=src/App.js

eavichay avatar Oct 21 '20 06:10 eavichay

fixed in #111

jfbrennan avatar Sep 05 '23 04:09 jfbrennan