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

Manually closing collapse #64

Closed
Aspie96 opened this issue Jul 2, 2016 · 35 comments
Closed

Manually closing collapse #64

Aspie96 opened this issue Jul 2, 2016 · 35 comments
Labels
enhancement potential improvement

Comments

@Aspie96
Copy link

Aspie96 commented Jul 2, 2016

Hi.

Is there any way to manually close a collapse created dynamically through:

var menuCollapse = new Collapse("#menu-button");

There is a .close(e) method, but it requires an event parameter.

Is there any way I can do it manually, asynchronously? It'd be really useful.
Thank you very much!

@thednp
Copy link
Owner

thednp commented Jul 3, 2016

Thanks for spotting that out, I will look into it and provide a solution asap.

@thednp
Copy link
Owner

thednp commented Jul 10, 2016

Sorry for the delay, I just improved this, but it doesn't work properly in IE8, currently working on a fix.

The problem is that it needs to know the target and the trigger inside the .close() and .open() method function bodies, so this would be a mess.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

How so?

The event is added through:

this.btn.removeEventListener('click', this.toggle, false);

Therefore, if I am getting this right (please, correct me if not), the trigger should always be self.btn (and you can easily get the target from there).

@thednp
Copy link
Owner

thednp commented Jul 10, 2016

Indeed you are wrong, the collapse instance does not know which one is self.btn until self.toggle is executed. I am going to try to find a way to simplify the script while keeping same visual and browser support.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

It looks it does, really.

When initializing each collapse, this.btn gets set to element (which is the button) and the event handler this.toggle gets first bind to such element.
toggle then sets it to self.getTarget(e).btn.
getTarget sets the return value btn to e.currentTarget. e.currentElement is at this point the original button because it is what has been clicked (since any other click would not have invoked the event).

So, whileas it's true that this.btn gets reset, it will actually be set to the same value each time.

I made a few tries with any collapse you have in the sapmple page (the menu one, the accordion and the one with two buttons), saving a pointer to the original button in a new field (myButton), and it is the case.
What I did was adding this.myButton = this.btn after the very first initialization of this.btn and then console.log(self.btn === self.globalButton); after every call of self.getTarget (only after which self.button might be modified) and it printed all trues.

I even removed every reintialization of self.btn and it works just fine.

@thednp
Copy link
Owner

thednp commented Jul 10, 2016

I have a new version of the file, un-minified only at this time. Would you like to test it in your app?
I also added a new feature, now the button's parent has an open class for further customization and usability.

The collapse .close() and .open() methods no longer require the event object as argument.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

I just downloaded the last version of collapse-native.js and repeated the two tests, getting the exact same results.

@thednp
Copy link
Owner

thednp commented Jul 10, 2016

Wait, I haven't committed yet.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

Ah, ok, I apologize.

Anyways, there was no need to modify it for this purpose as, as I said, "self.btn" never changes.

thednp added a commit that referenced this issue Jul 10, 2016
Also added a new feature for collapse to add 'open' class to the button's parent for further customization.
@thednp
Copy link
Owner

thednp commented Jul 10, 2016

OK now, go ahead and download, test and let me know.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

It does not work managing it manually.
I haven't checked why yet, but I would guess it is because of self.collapse not being set.

As self.btn never seems to change, nor the target does.
Therefore, this.collapse can easily be initialized in the Collapse constructor and getTarget is not actually needed.

@thednp
Copy link
Owner

thednp commented Jul 10, 2016

I was thinking the same while working on the code tonight, when I originally developed the script I was thinking of a solid solution for the use of accordions. I am going to try to change it now, and let you know.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

But it does work with accordion as well.

You create a brand new instance of Collapse for each single button (even if the accordion is the exact same).
So you have no reason at all to worry about accordions.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

You literally cycle throw all buttons (not collapsible items themselves) creating instances:

var Collapses = document.querySelectorAll('[data-toggle="collapse"]'), i = 0, cll = Collapses.length;
for (i;i<cll;i++) {
    var item = Collapses[i], options = {};
    options.duration = item.getAttribute('data-duration');
    new Collapse(item,options);
}

thednp added a commit that referenced this issue Jul 10, 2016
@thednp
Copy link
Owner

thednp commented Jul 10, 2016

The 'getTarget' method is needed in any case, we only know the btn as the element, we need to find out the target collapse. So there is a new commit, please download and try again.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

The 'getTarget' method is needed in any case, we only know the btn as the element, we need to find out the target collapse. So there is a new commit, please download and try again.

Yeah, yeah, of course.
I meant that it would only be executed once (so it could not be a method on its own, if one wanted to remove it, but it can stay there for simplicity).
But it does not need to change the button, at all, nor it ever needs a event handler.

@thednp
Copy link
Owner

thednp commented Jul 10, 2016

The new code doesn't change the btn and collapse on every open() or close() execution now, only once on init(). Please check in your app.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

Anyways, it is late here in Italy (01:10), so I am gonna quit thinking about JavaScript now, until tomorrow.

See you soon and thank you very much for your work.

If it helps you, I can modify the code myself, but not right now.
Just tell me if you want me to and I sure will.

Thank you again. See you tomorrow.

@thednp
Copy link
Owner

thednp commented Jul 10, 2016

Please test tomorrow the new code if you can do whatever you wanted in the first place.

@Aspie96
Copy link
Author

Aspie96 commented Jul 10, 2016

I sure will, thank you again.
Not now, since I couldn't even think rationally at 1:14. Not a good time for debugging, at least for me.

@Aspie96
Copy link
Author

Aspie96 commented Jul 11, 2016

Ok, I just tested it and it seems to work well and nicely.

Thank you very much again.

@Aspie96 Aspie96 closed this as completed Jul 11, 2016
@thednp
Copy link
Owner

thednp commented Jul 11, 2016

Thanks for your input, I appreciate it.

@RyanZim
Copy link
Contributor

RyanZim commented Jul 23, 2016

@thednp Can this be released soon?

@thednp
Copy link
Owner

thednp commented Jul 23, 2016

I believe so.
Perhaps you can go through your own tests and spot your own little thing, make sure to submit a PR.

@RyanZim
Copy link
Contributor

RyanZim commented Jul 23, 2016

Perhaps you can go through your own tests and spot your own little thing

Not sure what you mean.

@thednp
Copy link
Owner

thednp commented Jul 24, 2016

I mean do your own tests, maybe you find an issue with the current master, I hate deploying a new release and then right after a new stargazer finds a small little issue.

@RyanZim
Copy link
Contributor

RyanZim commented Jul 25, 2016

😀 It seems that's how it goes. I'll do a little work on the latest master and report back.

@thednp
Copy link
Owner

thednp commented Jul 25, 2016

Well, I think you are too late, I am gonna release 1.0.3 in a few minutes.

@thednp
Copy link
Owner

thednp commented Jul 25, 2016

Ok now, you can play around with the latest release.

@RyanZim
Copy link
Contributor

RyanZim commented Jul 25, 2016

@thednp npm publish please?

@thednp
Copy link
Owner

thednp commented Jul 25, 2016

I don't do that myself, I had a friend that did that, if you can contribute with that, I am happy.

@RyanZim
Copy link
Contributor

RyanZim commented Jul 25, 2016

@thednp It's not hard.

I assume you have NodeJS installed.

Open the terminal and navigate to the bootstrap.native directory. (Make sure git is at the commit that you tagged as v1.0.3). Type npm publish and hit enter!

If that throws an error, you will need to run npm login. This will prompt you for your email, username, and password for npmjs.com. (If you forgot your password, you can recover it at https://www.npmjs.com/forgot). Then run npm publish again.

I'm not familiar with bower, perhaps @IngwiePhoenix can help.
Edit: Bower runs off of the git tags, so that doesn't need anything done.

@thednp
Copy link
Owner

thednp commented Jul 25, 2016

I think I did it 💃 Please test.

@RyanZim
Copy link
Contributor

RyanZim commented Jul 25, 2016

🎉 You did!

@thednp
Copy link
Owner

thednp commented Jul 25, 2016

My first ever deploy to the npm.

@thednp thednp added the enhancement potential improvement label Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement potential improvement
Projects
None yet
Development

No branches or pull requests

3 participants