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

[Questions] - later added elements into the DOM: to be or not to be #102

Closed
thednp opened this issue Jan 15, 2017 · 107 comments
Closed

[Questions] - later added elements into the DOM: to be or not to be #102

thednp opened this issue Jan 15, 2017 · 107 comments
Labels
community question all in for feedback enhancement potential improvement help wanted need your suggestion

Comments

@thednp
Copy link
Owner

thednp commented Jan 15, 2017

For those who work with DOM manipulations and use the library or have any experience with JavaScript driven dynamic content, please reply:

  • Do you think we would need some tools and/or polyfills to enable our library to go LIVE (initialize proper BSN component for every new element added to the DOM) ?

  • If yes, what would you suggest? (I am currently experimenting with CustomEvent, MutationEvent and MutationObserver, working on a kind of polyfill for IE8)

  • About an IE8 polyfill, do you (or your scripts and tools) use mostly Element.prototype.appendChild, Element.prototype.innerHTML, both, or other methods?

To the most frequent contributors (please forgive me for tagging you)
@mathieuleclaire, @troyk, @mgiulio, @janpanschab, @RyanZim, @HelloGravity, @Jeff17Robbins, @DGT41, @oliseviche, @thewisenerd, @alzalabany, @kuus, @malexdev, @crcastle, @Aspie96, @CamWiseOwl, @antoligy, @zandaqo

AND anybody reading this page, please share your opinion.

@thednp thednp added community question all in for feedback enhancement potential improvement help wanted need your suggestion labels Jan 15, 2017
@kuus
Copy link

kuus commented Jan 15, 2017

Do you think we would need some tools and/or polyfills to enable our library to go LIVE (initialize proper BSN component for every new element added to the DOM) ?

I am not sure if bootstrap native uses event delegation but I seem to remember it does not, I think that could be enough. If I have to work with really dynamic pages I prefer using more robust frameworks.

About an IE8 polyfill, do you (or your scripts and tools) use mostly Element.prototype.appendChild, Element.prototype.innerHTML, both, or other methods?

Both

is out of topic but I would suggest to implement a modular es6 structure to the project (if I'll have time I'll try to work on that)

@thednp
Copy link
Owner Author

thednp commented Jan 15, 2017

is out of topic but I would suggest to implement a modular es6 structure to the project (if I'll have time I'll try to work on that)

We will work on bootstrap.native for Bootstrap 4 with this in mind, based on the code of my future commits.

The future BSN release will feature bootstrap original events, 24Kb minimized, a ton of improvements and new features. It would be something to consider, especially if I manage to get it done with the new features I have in mind.

@Jeff17Robbins
Copy link

Jeff17Robbins commented Jan 16, 2017 via email

@thednp
Copy link
Owner Author

thednp commented Jan 16, 2017

I am working on an CustomEvent for BSN for BS3 update.bs.dom, works in IE8+

document.addEventListener('update.bs.dom', function(e){
  // e.target is document

  // e.relatedTarget is the new/removed element

  // OR

  // e.boundElements [Array] IE8 with a newly added/removed element and it's children 
  // this event is only triggered when appendChild/removeChild methods are used
}, false)

When the event fires, there could be a handler to apply a proper BSN component via DATA API or something. This is probably suitable for BSN for BS3, but what do you think?

Indeed the ShadowDOM spec sounds very interesting.

@Aspie96
Copy link

Aspie96 commented Jan 16, 2017

I have been building a modified version of bootstrap.native that works even if you modify the DOM (without calling extra functions), but it is missing some elements.

Would it help if I published it on GitHub?

@thednp
Copy link
Owner Author

thednp commented Jan 16, 2017

@Aspie96 I believe it's an IE9+ solution?

@Aspie96
Copy link

Aspie96 commented Jan 16, 2017

I made it a while ago, but it should be IE8+.
I haven't tested it on actual IE8, though (the latest version of IE contains a simulator of older versions, I used that one).

@thednp
Copy link
Owner Author

thednp commented Jan 16, 2017

@Aspie96 please go for a fork of the current master and start applying your changes, I am curious if there could be something better than my current development.

Again, the BSN 1.5 series coming soon will have major code changes, for sure your version will not be mergeable with that, but at least we can all have an idea.

@Jeff17Robbins reading more into your input I can now say that everything you talk about is likely doable for the BSN for BS4, in combination with what @kuus was talking about. I am also interested in how you would want this to be implemented.

@Aspie96
Copy link

Aspie96 commented Jan 16, 2017

Forking is exactly what I did, but I cannot publish the project right now since I am not at home.

It is actuallyn quite a bit different from yours, but it is fully based on yours.

As I said, some controls are missing.

Maybe, before pushing on GitHub, I will paste the code on PasteBin (to fix a few things before pushing, like… the manual XD).
I hope you don't mind if I do so.

Thank you for using a free license such as MIT.

@Jeff17Robbins
Copy link

Here's a very crude, but working, example of packaging BSN Modal as a custom element. You need Chrome to see it run, because I didn't load the webcomponents polyfill.

https://jsfiddle.net/w1cmLc0g/

I call it 'crude' because it doesn't encapsulate everything about Modal, and it hasn't hidden the Modal's dialog contents in Shadow DOM, but it does define the toplevel aria and other attributes for you, showing how encapsulation can reduce user boilerplate.

<bsn-modal id="myModal">
  <div class="modal-dialog">
    <div class="modal-content">
      some content
    </div>
  </div>
</bsn-modal>

It takes care of that boilerplate plus instancing the BSN object in its connectedCallback:

  connectedCallback() {
    this.classList.add('modal');
    this.classList.add('fade');
    this.tabIndex = -1;
    this.setAttribute('role', 'dialog');
    this.setAttribute('aria-labelledby', 'myModalLabel');
    this.setAttribute('aria-hidden', 'true');
  
    this.modal = new Modal(this, {
      backdrop: 'true'
    });
    this.open = false;
  }

A fancier version would place the Modal's template in a <template> element, and use the <slot> element to parameterize the content of <bsn-modal>.

@thednp
Copy link
Owner Author

thednp commented Jan 17, 2017

As much as I like your example, it's too outer space for me, maybe you can shed some light for me: do the customElements behave like they are under MutationObserver, or something to enable dynamic content...

NO NO, let me ask you this: is there anything you can explain to me who I just heard of customElements and MutationObserver what is BEST to do, WHERE is BEST to go forth?

Thanks :)

@Jeff17Robbins
Copy link

Jeff17Robbins commented Jan 17, 2017 via email

@thednp
Copy link
Owner Author

thednp commented Jan 17, 2017

Why is that outer space?

It's a bit unusual for me, I still try to figure out ways to make components work in IE8+ and your examples blow me away so high... I never done this before, even if I knew there are some ways I can create custom elements, just I didn't investigate into that yet.

Anyways, we expect more people share their opinion in this topic, eventually we're getting somewhere.

@Jeff17Robbins
Copy link

Jeff17Robbins commented Jan 17, 2017 via email

@dgrammatiko
Copy link

@Jeff17Robbins I improved your code bit, for multiple modals: http://codepen.io/dgt41/pen/LxbzQG

@thednp
Copy link
Owner Author

thednp commented Jan 17, 2017

@Jeff17Robbins

So using custom elements for our library's components would make this library dynamic content enabled?

@dgrammatiko
Copy link

@thednp as I mentioned before, if I get some free time, I'm willing to create a Custom Elements fork of your library, @Jeff17Robbins is right CE are first class citizens for browsers and also the polyfill https://github.com/WebComponents/webcomponentsjs makes it compatible up to IE9(*)

Implementing your library as custom elements should be on your radar for BS4...

@Jeff17Robbins
Copy link

Jeff17Robbins commented Jan 17, 2017 via email

@thednp
Copy link
Owner Author

thednp commented Jan 17, 2017

In my current code, I changed the way components get initialized via DATA API, I have a new utility for that.

Additionally, I've setup a bulkInitializeDataAPI utility, haven't tested yet but it should work; the utility could be used for BSN3, I guess, as I'm reading the above may not be required for BSN4.

@Jeff17Robbins
Copy link

@DGT41 I moved the dialog markup to templates. http://codepen.io/anon/pen/PWbXwr?editors=1010 Not necessarily better, but illustrates using the <template> tag.

@thednp
Copy link
Owner Author

thednp commented Jan 24, 2017

@Jeff17Robbins, @DGT41
I've investigated how Bootstrap is handling events and more about the practice of delegating events to later added elements, they attach all event handlers to the document object and delegate to specific selectors. A practice jQuery developers themselves admitted it's a bad practice because that's too much of a burden on the document.

Instead, our way, we attach event handlers to each element on initialization, and if a modern browser detects you have deleted an element from the DOM, it will also clear the event handlers attached, which is a major plus.

Right now I have in mind a global click,scroll, etc handler attached to document but for all the components; this would save a huge ton of memory, not to mention the execution performance boost.

I am super curious about your ideas on that.

@thednp
Copy link
Owner Author

thednp commented Jan 26, 2017

I just commited a new version, please do some testing everyone ;)

@Jeff17Robbins
Copy link

I updated my custom element Modal example to the latest cdn version. It still works. I haven't dug in to see how the new event code works, but thought I'd post the example if it helps.

http://codepen.io/anon/pen/GrOYoO?editors=1010

@thednp
Copy link
Owner Author

thednp commented Jan 29, 2017

CDNjs is a bit behind, probably stuck in some semver related issue, here's the latest CDN
https://cdn.jsdelivr.net/bootstrap.native/2.0.1/bootstrap-native.min.js

I changed your pen with this one and it's not working, you must check the updated documentation on Modal.

I made some changes, I hope you get the point of the changes
http://codepen.io/thednp/pen/egeqJr

@Jeff17Robbins
Copy link

I'm confused. What is not working in http://codepen.io/anon/pen/GrOYoO?

@thednp
Copy link
Owner Author

thednp commented Jan 29, 2017

That isn't the latest CDN. Should be 2.0.1.

@Jeff17Robbins
Copy link

This is the line from http://codepen.io/anon/pen/GrOYoO? that pulls in BSN:
<script src='https://cdn.jsdelivr.net/bootstrap.native/2.0.1/bootstrap-native.js'></script>

Screenshot below. What isn't "2.0.1" about this? I'm confused because it says "2.0.1" in its path!

image

@gplume
Copy link

gplume commented Feb 12, 2017

@quantix-studio : Perhaps it is way beyond the topic here but here's a small example for a dropdown which doesn't include the click outside behavior but it's easy to add but slightly too overwhelming (like a @click="opened = false" on the largest div of the template, but why not if it's not too crowded in the DOM)
vuejs (*.vue) example:

<template>
<div class="dropdown" :class="{'open': opened}">
<button class="btn btn-default" @click="opened=!opened">--show dropdown menu--</button>
<transition name="fadeIO"> <!-- that's an improvement on the original BS! choose whatever transition you like -->
<ul class="dropdown-menu">
  <li @click="opened=false; otherFunctionCall()">...</li>
  <li @click.prevent="opened=false; otherFunctionCall()">...</li>
</ul>
</transition>
</div>
</template>
<script>
export default {
  data () {
    return {
      opened: false
    }
  }
}
</script>

This way you can also directly manage the persistent behavior if you need to with opened=false or not in the <li/> element...
Cheers!

@mathieuleclaire
Copy link

Hi,
sorry for intervening late in the discussion but it seems I have a problem linked to this thread with the use of popovers. I instanciate some popovers late -dynamically- in the DOM and they does not appear (the ones set at the DOM construction work fine). I tried with the 2.0.5 version for bootstrap 3.

Is it an expected behaviour ?
Thanks

@thednp
Copy link
Owner Author

thednp commented Feb 16, 2017

@mathieuleclaire this is my last reply on this regard: our library here is not jQuery powered and doesn't initialize on the fly for later added objects and also doesn't destroy instances for removed objects. In short it does nothing about any kind of DOM tree changes.

In this thread I am asking other developers for a better approach (better than original jQuery plugins for Bootstrap), as we haven't implemented anything here yet, we are still at the research phase.

I would suggest to create a constructor with a callback, and the callback should initialize your newly added elements or destroy removed elements.

@dgrammatiko
Copy link

@thednp check my approach for v4: https://dgt41.github.io/bs4-custom-elements/

@thednp
Copy link
Owner Author

thednp commented Feb 26, 2017

I was about to ask if anyone has done anything on this, so yea, lookin good.

@Jeff17Robbins
Copy link

Nice work @DGT41

@Aspie96
Copy link

Aspie96 commented Apr 14, 2017

Just in case someone wants to check it out too:

https://github.com/Aspie96/bootstrap.native.dynamic

It's in no way intended to be better than bootstrap.native or antyhing like that.
I am just sharing it just in case someone wants to check it out.

@thednp
Copy link
Owner Author

thednp commented Apr 14, 2017

Thanks for the heads up @Aspie96

@Aspie96 Aspie96 mentioned this issue Apr 19, 2017
@thednp
Copy link
Owner Author

thednp commented Jul 11, 2017

@jsdelivrbot npm bootstrap.native

@jsdelivrbot
Copy link

Browse the CDN files https://cdn.jsdelivr.net/npm/bootstrap.native/

Load the main file https://cdn.jsdelivr.net/npm/[email protected]
Make sure you configure the default file correctly. You can use "jsdelivr" or "cdn" in package.json to set the correct CDN file.

Version aliasing to latest major release https://cdn.jsdelivr.net/npm/bootstrap.native@2

Add .min. to files to auto-minify.

--
Full documentation

@Ixonal
Copy link

Ixonal commented Sep 13, 2017

Out of curiosity, was any reason given for initialization via delegated events being bad? Isn't event delegation considered a good thing?

I'm also a little leery of the custom element approach, if only because this is, as far as I can tell, intended to be a drop-in replacement of existing Bootstrap behavioral code. Requiring that people now use custom elements would break that and have pretty major impacts on anyone trying to switch from regular Bootstrap to bsn.

@thednp
Copy link
Owner Author

thednp commented Sep 13, 2017

@Ixonal I cannot find the forum post where people talk about poor performance of delegated events, it's basically a way jQuery does (did) in dealing with everything you throw at it at the cost of performance. Don't get me wrong, jQuery does a ton of things right, in a very elegant and uniform manner, just that the demand for better performance always wins in the end.

Custom elements seems to be the next generation content delivery dynamics, I have no doubt about it now, my code should work just fine with it regardless, as shown here. (I updated the fiddle).

It's a matter of taste and application, it's pointless arguing which way is best, any library can be very suitable for many use cases.

@Ixonal
Copy link

Ixonal commented Sep 13, 2017

@thednp I could see there being delegation performance issues if there are a great deal of handlers (and I mean an absurd amount) that are tied to selectors (instead of specific elements), as that would run the Sizzle selector engine a great many times, which would be horribly inefficient. Adding one delegated handler per component type (to init the component), however, should not add any appreciable overhead.

In the way of custom elements, I'm not arguing their usefulness or that they're the way web development is moving. My argument is that Bootstrap presented a certain interface. Your library purports to adhere to that interface, but moving to using custom elements breaks that contract. Were Bootstrap to use custom elements, and your library moved to match, I would have no issue.

@Ruffio
Copy link

Ruffio commented Jan 3, 2018

what is the status of this issue?

@thednp
Copy link
Owner Author

thednp commented Jan 3, 2018

@Ruffio the issue is still open.

@pavelloz
Copy link

pavelloz commented Jan 12, 2018

Hello :)

My view is this:

  1. Personally I think this is not the components library job to observe DOM and react for new elements - it is a developer job to know where to apply appropriate observers and react. Imagine that you do an observer and a developers adds 1000 elements, one every 5ms - the event handler will fire a lot. Developer manually probably will add some kind of debouncing - flexibility for end user is huge by not doing it at all.

  2. If you decide to do something about it - I totally agree with @Ixonal on this one. Having 10 event handlers (1 per type) that will react if newly added dom element is in fact type x wouldn't be a big deal at all. It is the check of a element type that will me more expensive, I suppose. Additionally, I believe it can be done using one event handler on the document, just need to come up with a good (fast & robust) selector to catch all the components provided by the library.


Someone mentioned moving to es6 - I also agree. That would add one little step to the build process (babel) and also allow having multiple builds (babel-env) for legacy browsers, modern browsers - some library users know for a fact that they just don't care about old browsers (ie. private projects) and would like to shave off some boilerplate code and see pretty shiny es6 in the dev tools :)


I'm adding this repo to my favorites and I'll try to observe its development just in case I have time to help - you are doing a good job that should have been incorporated into official TWBS IMO, I don't know why it hasn't, such a great initiative (I've been dreaming about it for a long time).

Cheers, Pawel

@torosm
Copy link

torosm commented Feb 2, 2018

Yes = to be.

@tamb
Copy link

tamb commented Jun 28, 2018

I am not in favor of this. If we add more components to the DOM we should be responsible for wiring up their javascript.

@TwanoO67
Copy link

Personnaly my answered would have been quite simple:

" As it's working dynamically in the original twitter bootstrap, this should be also implemented here. "

If not, using this library is simply awfull in a SPA.
example in vuejs2, each time I have a "v-if" property (that could remove/re-add a element from dom)
I have to call, the initCallback on the element again.
So I have to explicitly watch all my properties that could act on a v-if.
It's just too much mess to add to components, and it totally ruined the use of this library.

At least, it should exist an option, to have let the library being dynamic.

@thednp
Copy link
Owner Author

thednp commented Aug 31, 2018

@TwanoO67 if you would be so kind as to come with an idea on how to implement it would make a better world, instead you come with a "your library is awfull in SPA".

@TwanoO67
Copy link

Sorry @thednp it wasnt meant to be hurtfull.
I'm not a native English speaker so maybe my words sound false.

The idea was the library is good and it's nice to not depend on jQuery and so ever.
But without this functionality the use of this library in a spa is very difficult to understand for starter user.

As I understand there was plenty of implementation proposal in the previous messages. So I don't think that mine would be of any help ( as it is based on mutation observer as previously proposed). The point was just that, as lot of people is needing it, the implementation you thing the most "proper" one should be proposed in the lib

@MattCCC
Copy link

MattCCC commented Feb 16, 2019

Hello, firstly thank you for a great lib! It's really helpful for me. Regarding the issue, imho basically the best (and easiest) solution to make it fully compatible with BS4 without any troubles would be doing exactly what they currently do - event delegation. It's currently simplest solution to this problem.

@subversivo58
Copy link

subversivo58 commented Feb 9, 2021

I know a little late for the party, but I've been following this issue for a little over a year and with no news for now.

In version 3.0.0 I made modifications to meet my dynamic loading needs and add ES6 export see link: https://github.com/subversivo58/misc/blob/master/js/bootstrap.native.mjs

Now I am migrating to Bootstrapv5 and I have returned to the question of support for dynamic elements.

In the current version (3.0.14c) I added only two auxiliary functions and use Mutation Observer to track" mutations "in the document tree.

The use of async/await was necessary because apparently calling initCallback() from .forEach() was not being able to apply.

The modification itself:

// before `export`
// helpers
function isUndefined(arg) {
    return arg === void 0;
}
function isElement(obj) {
    try {
        return (obj.constructor.__proto__.prototype.constructor.name) ? true : false;
    } catch(_) {
        return false;
    }
}

// Options for the observer (which mutations to observe)
let config = { childList: true, subtree: true };

// Callback function to execute when mutations are observed
let callback = function(mutationsList) {
    for(let mutation of mutationsList) {
        if (mutation.type === 'childList') {
            ;[...mutation.addedNodes].forEach(async addedNode => {
                if ( isElement(addedNode) && !isUndefined(addedNode.querySelectorAll) ) {
                    await initCallback(addedNode, true);
                }
            })
        }
    }
};

// Create an observer instance linked to the callback function
let observer = new MutationObserver(callback);

// Start observing the target node for configured mutations
observer.observe(document, config);

I would like to hear some thought about the usefulness of this in this library since async and the await operator already have more than 90% in the main browsers except (obviously) for IE.

See: https://caniuse.com/async-functions and https://caniuse.com/mdn-javascript_operators_await


PS: MutationObserver() support: https://caniuse.com/mutationobserver

@thednp
Copy link
Owner Author

thednp commented Feb 9, 2021

Well this is part of the init of our library, I think it's open to any and each developer to create his/her own implementation. If MutationObserver is the best for you, go for it! You create your own custom build with your own init.js part and you're all right.

As other people mentioned above, CustomElement is also a viable solution, so implementing one in favor of the other would certainly have its side effects. Others prefer our implementation because it's so easy to do turbolinks:load or just because it's actually more snappy and pleaseant experience.

I'm still not considering delegating document.addEventListener to every click just to init BSN.Button it's ridiculous to have document.querySelectorAll('[data-bs-toggle="button"]') and many others, on every single click event.

All in all, I'm more in favor of having the issue open and allow devs like yourself to contribute, share ideas and implement something beneficial to everyone.

In my mind we could have several versions of init.js

In the end every solution is suitable for a specific case. What's on your mind?

@subversivo58
Copy link

In my mind we could have several versions of init.js

In the end every solution is suitable for a specific case. What's on your mind?

I have no experience with Custom Elements but I like the idea of having versions of init.js for custom builds

@thednp
Copy link
Owner Author

thednp commented Jun 9, 2021

@subversivo58 you can actually do that already, if you check some wiki of this project you can figure out for yourself.

I think it's time to put this issue to rest, I've seen Custom Elements become more widely adopted and eliminate the need for a work around our issue at hand.

Thanks to everyone contributing.

@thednp thednp closed this as completed Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community question all in for feedback enhancement potential improvement help wanted need your suggestion
Projects
None yet
Development

No branches or pull requests