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

Reuse of DOM nodes instead of HTML based storage #145

Open
Danielku15 opened this issue Apr 26, 2018 · 10 comments
Open

Reuse of DOM nodes instead of HTML based storage #145

Danielku15 opened this issue Apr 26, 2018 · 10 comments

Comments

@Danielku15
Copy link

We are considering to buy a commercial license of this library. But there is one major issue that currently prevents us from using this library instead of building our own system: When you initialize Clusterize on a existing element, it copies the HTML of the nodes instead of reusing the already created nodes.

We are currently updating the DOM of our list items with some data from an API depending on the user selection. But if we update the initially created DOM elements, clusterize does not pick it up as it cloned over the HTML during initialization.

Is there a chance that this library will be extended to reuse the existing DOM nodes instead of utilizing HTML cloning.

@NeXTs
Copy link
Owner

NeXTs commented Apr 26, 2018

Hello

I don't think so because of memory allocation required for storing actual DOM nodes
read #80 (comment)

@Danielku15
Copy link
Author

Sorry that I did not see the pull request. I was only looking in the issues for similar topics but could not find anything or I was blind 😄 .

Of course there are pros and cons for both systems and I think I was not clear with my question: I would prefer to have an option that allows switching me between HTML and DOM mode. We have other usecases where also HTML storage would be perfectly fine. But on the page where we need to dynamically maniuplate the DOM of the not shown elements it would be nice to change the behavior.

I am aware of the bigger memory load in this case but it would be an unrealistic effort to rework our list that it would be HTML serializable. Our list items have interactive elements like buttons that open dialogs, progress bars and labels that show values that are dynamically updated via API calls and also items are selectable via checkboxes. All this functionality is broken and would work if DOM elements are stored.

The virtualization via Clusterize.js should exactly solve the issue that due to the complex DOM the page becomes laggy with too many elements.

@NeXTs
Copy link
Owner

NeXTs commented Apr 26, 2018

You wouldn't be able to update DOM nodes while they are in memory.

If this is ok for you, try this fork
https://github.com/STRd6/Clusterize.js

@Danielku15
Copy link
Author

You wouldn't be able to update DOM nodes while they are in memory.
Why not? If the DOM nodes are just not in the tree but still alive, you can manipulate the DOM nodes if you still know them if you have them in some lookup or list. Like:

// create list element
var myList = document.createElement('div');
myList.classList.add('clusterize-scroll');

var content = document.createElement('div');
content .classList.add('clusterize-content');
myList.appendChild(content );

// create some items
var items = [];
for(var i = 0; i < 1000; i++) {
    var item = document.createElement('div');
    content .appendChild(item);
    items.push(item);
}

// initialize clusterize
var clusterize = new Clusterize({
    scrollElem: myList,
    contentElem: content
});

// manipulate elements afterwards
for(var i = 0; < items.length; i++) {
    if( ( i % 2) == 0 ) items[i].innerText = i;
}

If I'd file a pull request with the required changes (similar to #80 but with an option) is there a chance you will merge it?

@NeXTs
Copy link
Owner

NeXTs commented Apr 26, 2018

oh so you want to edit items directly on list.
I wasn't sure how would you identify (search) needed DOM tags in list.
In demo you've used if( ( i % 2) == 0 ) hence I understand it may be simplified..

But how often do you need to update? You can still update strings this way

var content = '<div class="class-name">content</div>' // items[i]
var helper = document.createElement('div')
helper.innerHTML = content
var parsedContent = helper.firstChild
parsedContent.setAttribute('custom', true)
items[i] = parsedContent.outerHTML

@Danielku15
Copy link
Author

Whenever the user makes a change on the page (selecting new records in the list, adding new inputs in the other UI elements etc.) we make an API call to reload some details per record. Then we do several updates on each list item:

  • Set some CSS styles (for a progress bar width)
  • Update some textual things.
  • Highlight some sub elements under conditions.

Maybe updating could be achieved, but parsing and serializing seems not a very performant solution. Also we will loose a lot other features that we currently initialize on our list:

  • we store some model object in the DOM to access it later on. We could try to also rewrite data attributes but that sounds like a lot of effort.
  • If the user clicks on a button in the list, a dialog specific for the item is opened. Click listeners are lost. We could try to rebuild it with bubble events on the parent.
  • We have a checkbox and also if you click on the whole row you can "select" the element. Again bubble events could solve this.

But all changes are more a workaround to rebuild compatibility with Clusterize.js. For such use cases is not simply "initialize it and it works". You need to rebuild your application that it still works.

You might have seen my PR. I already integrated this version and it works perfect for our usecase. We can update our items any time and they are shown correctly on scrolling and all event listeners are working too. No extra effort on our code is needed anymore, we initialize the plugin and it runs. 😁

I'm not sure about the licensing but I guess if we will buy the commercial license we are also allowed to make any adjustments to the script. But maintaining our own fork just for this feature is quite an effort. It would be great if this feature becomes part of the official plugin.

@NeXTs
Copy link
Owner

NeXTs commented Apr 26, 2018

Since your fork works for you, use it. No worries on licensing.
I will review PR as soon as I have more free time.

@Danielku15
Copy link
Author

Yes, we will use the fork for now. Simply let me know if you have doubts or change requests to the PR.

For the future it would be great if our changes find it's way back to the official project. This way others can also benefit and also in future we can upgrade to newer versions of Clusterize.js without worries.

@Danielku15
Copy link
Author

It has been quite a while since I opened this issue and also provided the fix for it. Any chance that this change finds its away into the official version? We plan to move in our project more to package managers and would like to pull the library from cdn.js. 😃 For this library it would not be possible as we currently still rely on the fork.

@dumblob
Copy link

dumblob commented Oct 24, 2021

@Danielku15 if there is any "warranty guarantee" when using the paid version, you might try that path.

Or if you're maintaining your fork, do you actually need to keep the dual licensing? I think only GPL mandates keeping the license, so it's probable that the commercial license is not any more needed if you're using your fork. In such case you could publish your fork to npm or whatever package repository your package managers are using without being bound to this project.

Disclaimer: I don't think this is bad behavior in any way - it's just solving your downstream issue in a situation when upstream doesn't cooperate for a year or longer.

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

No branches or pull requests

3 participants