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

Added shown and hidden events. #81

Closed
wants to merge 1 commit into from
Closed

Added shown and hidden events. #81

wants to merge 1 commit into from

Conversation

threax
Copy link

@threax threax commented Sep 26, 2016

Added the shown.bs.modal and hidden.bs.modal events to the modal dialog.

@thednp
Copy link
Owner

thednp commented Sep 26, 2016

Sorry, no custom events except the carousel. If you want this to go live, please open an issue and ask some opinions, most people don't need these events. Thank you.

@thednp thednp closed this Sep 26, 2016
@kuus
Copy link

kuus commented Dec 2, 2016

@thednp can you explain why you would not add these events? How do you fire a callback when a modal is close without them? A modal can be closed in several ways, so it certainly makes no sense to attach listeners e.g. to close and cancel buttons and to the esc key. Furthermore, this PR uses the same technique used in the carousel and it's just 6 lines of code

@thednp
Copy link
Owner

thednp commented Dec 2, 2016

@kuus

In fact it's about less lines, but that's not the point. The idea is to keep things to as minimal as possible so that everyone can fork and adapt to so many needs.

I am also not against this, just to make sure I make things clear, we have to take decisions in the best interests of the entire community and not just a group of people, which means I wish we have more people involved and express their opinions in these changes.

@kuus
Copy link

kuus commented Dec 2, 2016

I see. But, isn't the point of this project to provide Bootstrap functionalities without js dependencies? It seems to me that the modal component without events is very limited comparing to Bootstrap's one. Having said that I forked my version, IMHO it's such a basic feature that I think it should be included

@thednp
Copy link
Owner

thednp commented Dec 2, 2016

Give me 5 more opinions and the next version will have it.

@dgrammatiko
Copy link

dgrammatiko commented Dec 2, 2016

One more 👍 from me as well!

PS I already had asked for this few months ago...

@thednp
Copy link
Owner

thednp commented Dec 2, 2016

I know @dgt41 I hope you and the guys here aren't mad about this.

Also I wish to know from everyone all the components we need to add this functionality. Currently we have carousel, modal is on the topic, dropdown, tabs and collapse might be of interest perhaps.

@dgrammatiko
Copy link

Oh, well we can't be mad about your excellent code! Though, as I said before, for modals some event or callback is needed if you want to use it with iframes (and reload the iframe on open, destroy it on close).

@thednp
Copy link
Owner

thednp commented Dec 2, 2016

As a proposed draft, I was thinking about a new utility to help us quickly register and dispatch events

// definition
var bootstrapCustomEvent = function(eventName,component){
  if (('CustomEvent' in window) && window.dispatchEvent) {
    this.modal.dispatchEvent(new CustomEvent(eventName+".bs."+component));
  }
};

// usage
bootstrapCustomEvent.call(this,'shown','modal'); // or bCE.call(self,eventName,component) if `this` is not same with `self`

Do you think this would make sense?

@dgrammatiko
Copy link

💯

@kuus
Copy link

kuus commented Dec 2, 2016

As a proposed draft, I was thinking about a new utility to help us quickly register and dispatch events

what if the element itself is passed as the context to the utility? Doing so it can be reused throughout all the various bootstrap components.

// definition
var bootstrapCustomEvent = function(eventName, componentName) {
  if (('CustomEvent' in window) && window.dispatchEvent) {
    this.dispatchEvent(new CustomEvent(eventName+".bs."+component));
  }
};

// usage
bootstrapCustomEvent.call(this.modal, 'shown', 'modal');

@dgrammatiko
Copy link

@kuus isn't this in bootstrapCustomEvent.call(this,'shown','modal'); referring to the caller instance, in this case Modal?

@thednp
Copy link
Owner

thednp commented Dec 2, 2016

@dgt41 that's right. That would leave room for the utility to get all the juice of the instance.

@kuus
Copy link

kuus commented Dec 2, 2016

@dgt41 I was imagining to put that bootstrapCustomEvent as a sort of static method in the Utils.js file so that it can be reused in the carousel or wherever else

@thednp
Copy link
Owner

thednp commented Dec 2, 2016

@kuus go ahead and test in your environment, test both see what's best for you, let us know.

@kuus
Copy link

kuus commented Dec 2, 2016

@thednp
Copy link
Owner

thednp commented Dec 2, 2016

I think it looks good, now the question is: does it work 100% perfect in all IE8+ browsers with our polyfill/minifill?

Also what about other components like collapse, tabs, alert, etc?

A word of advice: I warn you NOT to do anything about scrollspy and affix, leave them alone.

@kuus
Copy link

kuus commented Dec 2, 2016

I think it looks good, now the question is: does it work 100% perfect in all IE8+ browsers with our polyfill/minifill?

I can't test on IE at the moment, but if it was working before it should be the same now

Also what about other components like collapse, tabs, alert, etc?

We could add events to those as well in another moment. I could prepare a pull request next week

A word of advice: I warn you NOT to do anything about scrollspy and affix, leave them alone.

Fine with that!

@RyanZim
Copy link
Contributor

RyanZim commented Dec 2, 2016

👍 For adding events to modal.

@thednp
Copy link
Owner

thednp commented Jan 8, 2017

I finished adding events to ALL components (most as the original components events) as well as a ton of improvements. The new BSN is gonna be at least 8K smaller size :)

Can't wait for your opinions and test results.

@dgrammatiko
Copy link

@thednp can't wait for the commit ;)

@thednp
Copy link
Owner

thednp commented Jan 12, 2017

Hey I started to rename some methods, some will be same as the original plugins, some for better readability, do you think users will mind this?
Of course the release changelog will mention all changes.

@dgrammatiko
Copy link

dgrammatiko commented Jan 12, 2017

Personally I will be able to manage those renamed functions (hopefully) but as a general rule of thumb if there are backwards incompatible changes, according to semver, there should be a major release e.g. 2.0.0

@thednp
Copy link
Owner

thednp commented Jan 12, 2017

How about 1.5?

@dgrammatiko
Copy link

That would be fine as well

@thednp
Copy link
Owner

thednp commented Jan 12, 2017

All right then. Man a lot of stuff to document.

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

Successfully merging this pull request may close these issues.

None yet

5 participants