search

jarek-foksa / xel

Xel - Widget toolkit for building native-like Electron and Web apps
https://xel-toolkit.org
Other
691 stars 59 forks source link

Fix breaking virtual dom by x-select element #35

Closed dimapaloskin closed 3 years ago

dimapaloskin commented 7 years ago

Current version сauses error after first select.

I tested it with hypperapp, but I think it is rightly for react too.

This happens because x-overlay is removed from real DOM by .hide method, so renderer lost reference on real node.

This PR should fix this problem.

What do you think about it?

dimapaloskin commented 7 years ago

So I can see that x-overlay has own tricky logic, so my PR need works :)

dimapaloskin commented 7 years ago

@jarek-foksa if we are will change .remove to .style.visibility = 'hidden' it can has unexpected side-effects?

https://github.com/jarek-foksa/xel/blob/a6a6f22bb19e1671f4cfcba7510b97032fd83d3d/elements/x-overlay.js#L125

jarek-foksa commented 7 years ago

Yes, it is going to break things. Note that <x-overlay>.show() takes care of correct positioning of the overlay and prevents document from being scrolled. Ideally the overlay should be kept inside the shadow DOM of <x-select>, but this turned out to make it impossible to position it correctly, so I had to resort to the current hackish solution where I'm inserting the overlay before <x-select> temporarily.

Is this how all frameworks that use virtual DOM behave? I.e. do they break when elements are added to or removed from the rendered DOM tree by means other than the framework-specific render method?

dimapaloskin commented 7 years ago

Not sure about other frameworks. Unfortunately I couldn't to make friends with xel and create-react-app (also create-preact-app). Will test it.

dimapaloskin commented 7 years ago

There are very strange things. I tried few boilerplates with xel (I did setup it based on Setup instructions) and with each boilerplate I can see infinity bundle loading:

Gif

Any ideas why it happens?

dimapaloskin commented 7 years ago

Yes, it is going to break things. Note that .show() takes care of correct positioning of the overlay and prevents document from being scrolled.

Is my PR breaking this functionality? It's just set visibility = hidden to overlay, instead really removing it from DOM.

jarek-foksa commented 7 years ago

If you just replace this.remove() with this.style.visibility = "hidden" in <x-overlay>.close() then the overlay might make some other widgets non-clickable even after it has been closed. You should probably use this.style.display = "none" instead.

I haven't made the final decision yet, but I'm thinking about focusing this project exclusively on vanilla HTML/DOM/JS/CSS development. This would mean I would ignore any bugs that can be reproduced only with some specific framework.

dimapaloskin commented 7 years ago

If you just replace this.remove() with this.style.visibility = "hidden" in .close() then the overlay might make some other widgets non-clickable even after it has been closed. You should probably use this.style.display = "none" instead.

Make sense. Will try it.

I haven't made the final decision yet, but I'm thinking about focusing this project exclusively on vanilla HTML/DOM/JS/CSS development. This would mean I would ignore any bugs that can be reproduced only with some specific framework.

Ok :)

jarek-foksa commented 3 years ago

Since Xel 0.9.0 is now dual-licensed under GPLv3 and commercial license I won't be able to accept pull requests anymore. If you can still reproduce the original issue, please report it on https://xel-toolkit.org/issues.