thednp / bootstrap.native

Bootstrap components build with Typescript
http://thednp.github.io/bootstrap.native/
MIT License
1.7k stars 177 forks source link

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

Closed thednp closed 3 years ago

thednp commented 7 years ago

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

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.

Jeff17Robbins commented 7 years ago

I might have time to work on this more tomorrow.

On Mon, Jan 30, 2017 at 7:12 PM thednp notifications@github.com wrote:

@Jeff17Robbins https://github.com/Jeff17Robbins how do you link a trigger and a modal together? How would you write it so that it can satisfy us all?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thednp/bootstrap.native/issues/102#issuecomment-276233769, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAWh97ACpeAK3VgJXzq4DA5BB9A_k5Vks5rXnxQgaJpZM4Lj-Tp .

dgrammatiko commented 7 years ago

@Jeff17Robbins I've kept the id and the data-target in order to keep the markup as close to the original (bootstrap) as possible. Rethinking this, probably, we've could do it differently, but it might need also different markup and that will make the migration from the original bootstrap to the custom elements a bit harder...

Jeff17Robbins commented 7 years ago

Does the original bootstrap require passing the trigger element (e.g. the <button>) into new Modal(...)?

As best as I can see at http://getbootstrap.com/javascript/#via-javascript, it does not.

We can leave the id on the dialog, and use data-target on the trigger.

I am requesting that bootstrap native drop that argument from new Modal(...). It is an extra coupling that is not required.

thednp commented 7 years ago

Does the original bootstrap require passing the trigger element (e.g. the

No, and I am working on our component to do the same now. This would mean that you can no longer have multiple initialization instances on same modal at the same time, as it works right now with BSN, but that's also subject to change.

thednp commented 7 years ago

To @Jeff17Robbins OK Now. I managed to make it work like the original, you can also initialize on a modal element and totally ignore a triggering element, while having full control over the methods via JavaScript.

The DATA API will initialize all buttons with data-toggle="modal" and data-target="#modal-ID" or href="#modal-ID", but via Javascript you have full control over the initialization, including the ability to initialize same element multiple times without re-adding specific event listeners over and over.

Updating docs and preparing to commit asap. I will need your help with testing.

Jeff17Robbins commented 7 years ago

Sounds awesome! I should have time tonight to try it out. Which CDN is it on?

thednp commented 7 years ago

@Jeff17Robbins you should rely on the demo, use this file (when it turns to version 2.0.2) for your testing. We don't push to CDN and npm unless we have it all tested and documented.

thednp commented 7 years ago

UPDATE:

@Jeff17Robbins https://github.com/thednp/bootstrap.native/commit/69f67ccf7ee3ae35dacfb36c658565ccc8b8f517

thednp commented 7 years ago

Well @Jeff17Robbins ?

Jeff17Robbins commented 7 years ago

Here's an update to a custom element (note to @dgt41 , using the v1 API customElements.define(...))

http://codepen.io/anon/pen/MJVgpq

I put the trigger (<button>) inside the custom element.

<bsn-modal>
  <button class="btn btn-default btn-lg">Toggle modal</button>
  <main>
  <div class="modal-dialog">
    <div class="modal-content">
      some content
    </div>
  </div>
  </main>
</bsn-modal>

The implementation of <bsn-modal>:

class BsnModal extends HTMLElement {
  constructor() {
    super();
  }
  connectedCallback() {
    this.setAttribute('role', 'dialog');

    var main = this.querySelector('main');
    main.setAttribute('aria-hidden', 'true');
    main.classList.add('modal');
    main.classList.add('fade');
    main.tabIndex = -1;

    this.modal = new Modal(main, {
      backdrop: 'true'
    });

    this.trigger = this.querySelector('button');
    this.click = (evt) => {this.modal.toggle();}
    this.trigger.addEventListener('click', this.click);
  }
  disconnectedCallback() {
    this.trigger.removeEventListener('click', this.click);
  }
}
customElements.define('bsn-modal', BsnModal);

I managed to have zero ids, because the trigger is inside the custom element. Putting the trigger inside the custom element makes it the custom element's problem to find the trigger.

Right now, the custom element code is weak in that it expects the trigger to be the first <button> it finds inside itself with this.querySelector('button'). That code could be made stronger, without needing an id that gets in the way of modular custom elements.

thednp commented 7 years ago

@Jeff17Robbins There is something I don't understand: why you set via JavaScript backdrop: 'true', it should be either backdrop: true or backdrop: 'static', the option is true by default.

Jeff17Robbins commented 7 years ago

Just my ignorance, sorry.

Aspie96 commented 7 years ago

@thednp, Sorry for being late: http://pastebin.com/DQYCBBt7

thednp commented 7 years ago

@Aspie96 indeed late, but still, give the new master a try and tell us what you think.

UPDATE:

Looking at your code, you're doing exactly what the original plugins & jQuery do, probably you're using the same code with the document.on() event delegation utility. jQuery developers themselves admitted it's a bad practice, we're looking to find new ways around those bad practices, and the above comments prove to dictate the roadmap for future versions of BSN.

Aspie96 commented 7 years ago

Looking at your code, you're doing exactly what the original plugins & jQuery do, probably you're using the same code with the document.on() event delegation utility. jQuery developers themselves admitted it's a bad practice, we're looking to find new ways around those bad practices, and the above comments prove to dictate the roadmap for future versions of BSN.

Well, thank you for letting me know this.

Indeed, what I did is not equally useful to what you did, at all. But I think it can be a useful alternative when you want to use Bootstrap as it is, just without jQuery, but also without writing extra code and not being able to simply edit the page content. Of course I am talking about the current version of your code, not the future ones.

probably you're using the same code with the document.on()

Am I really? I wrote it on my own, so it would be pretty odd if it was the same code.

Anyways, I apologize if I am not being helpful and I am not following the discussion. I am having some university exams, also. So I apologize and I thank you again for your library.

thednp commented 7 years ago

@Aspie96 best of luck with the exams.

Aspie96 commented 7 years ago

Thank you, man!

thednp commented 7 years ago

So everybody your tests alright? I wanna go with a new release tag and get some work on something else.

Jeff17Robbins commented 7 years ago

sounds good

On Wed, Feb 1, 2017 at 5:50 PM thednp notifications@github.com wrote:

So everybody your tests alright? I wanna go with a new release tag and get some work on something else.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/thednp/bootstrap.native/issues/102#issuecomment-276808958, or mute the thread https://github.com/notifications/unsubscribe-auth/ACAWhwkuOg0ww-uuLc3PYWPnPgzohd2-ks5rYQwmgaJpZM4Lj-Tp .

gplume commented 7 years ago

A big +1 to the original post. I really need this functionality! If I understand well we just have to wait testing/doc-writing and it will be added to the next release? Thanks for the great work and for saving some internet bandwidth :)

thednp commented 7 years ago

@gplume which one are you reffering to?

gplume commented 7 years ago

"...later added elements into the DOM" ! for bs3 btw.

thednp commented 7 years ago

@gplume that is something we already discussed in this very thread, if you read careful, you will find out it's a very bad practice to bind all event handlers to the document in order to handle later added elements.

For that reason, BSN v4 will feature something with customElement to deal with later added elements.

quantix-studio commented 7 years ago

hi @thednp and thanks for the reactivity !

I'm using bsn's v4 dist on a vuejs project and am confronted to that issue too.

I'm trying to figure out if bsn will do the job in time, so here are my questions : have you an idea of the customElement availability delay ? is a modular (requirable) version in project ? can my team and I help in the reflexion ?

Big thank again for your efforts and reactivity, performance is the way ! ;)

thednp commented 7 years ago

@quantix-studio please read this https://github.com/thednp/bootstrap.native/issues/102#issuecomment-273203567

and all @Jeff17Robbins messages

dgrammatiko commented 7 years ago

@quantix-studio if you are using vue then why not use https://yuche.github.io/vue-strap/ ?

quantix-studio commented 7 years ago

@dgt41 no support for vuejs2 AND twbs4 at the moment :(

gplume commented 7 years ago

Same use case than @quantix-studio but I've found, reading BSN code, that it's quite easy to reproduce the same behaviors with the framework itself, I use vuejs2 and angular2. The only thing I didn't found is the ability to close a dropdown clicking outside the item... We have to rewrite all transitions effects (tabs fade or modal slide) though and it can be a lot to do but soon you prepare all your classes for that, even write better effects and then it's just a matter of reusing them. My first reaction was based on some database content that had a collapse item included. Bad practice, I wrote the element elsewhere and have now a component that display those tables into a collapse button outside the main text. Sometimes (often?)limitations force you to do better things (like mentioned elsewhere in this discussion!

thednp commented 7 years ago

The only thing I didn't found is the ability to close a dropdown clicking outside the item...

This is incorrect, check the demo http://thednp.github.io/bootstrap.native/#dropdownExamples

gplume commented 7 years ago

I meant that by not using this library but just vuejs or angular2. Your demo and general behavior is fine.

quantix-studio commented 7 years ago

Hi @gplume. Yes indeed, quite a lot of work. Would you mind sharing your generic code ? I'd be so grateful quantix.studio@gm@il.com

gplume commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

thednp commented 7 years ago

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

Jeff17Robbins commented 7 years ago

Nice work @dgt41

Aspie96 commented 7 years ago

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 commented 7 years ago

Thanks for the heads up @Aspie96

thednp commented 7 years ago

@jsdelivrbot npm bootstrap.native

jsdelivrbot commented 7 years ago

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

Load the main file https://cdn.jsdelivr.net/npm/bootstrap.native@2.0.12 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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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 commented 6 years ago

what is the status of this issue?

thednp commented 6 years ago

@Ruffio the issue is still open.

pavelloz commented 6 years ago

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 commented 6 years ago

Yes = to be.

tamb commented 6 years ago

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 commented 6 years ago

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.