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

Any instance of the string 'in' in a collapse's class attribute breaks Collapse #54

Closed
lcannon opened this issue Apr 21, 2016 · 25 comments
Labels
enhancement potential improvement

Comments

@lcannon
Copy link

lcannon commented Apr 21, 2016

Presumably this would be fixed by changing the regex to match /\bin\b/ instead of /in/ in the toggle function.

@thednp
Copy link
Owner

thednp commented Apr 21, 2016

Do you have a test scenario with the issue please?

@lcannon
Copy link
Author

lcannon commented Apr 21, 2016

Sure. The situation that brought this up was a collapse

<button type="button" class="navbar-toggle" data-toggle="collapse" data-target=".navbar-warning-collapse">

that targeted this

<div class="navbar-collapse collapse navbar-warning-collapse">
    ...
</div>

Because /in/ matches against 'navbar-warning-collapse' the toggle method always tries to close the collapse.

@lcannon
Copy link
Author

lcannon commented Apr 21, 2016

Thanks for the library, by the way, was exactly what I was looking for!

@thednp
Copy link
Owner

thednp commented Apr 22, 2016

Does the \bin RegExp work with IE8? I will probably have to update all code to this, I didn't expect other cases could be touched.

@thednp thednp closed this as completed Apr 22, 2016
@thednp thednp added the enhancement potential improvement label Aug 22, 2016
@thewisenerd
Copy link

thewisenerd commented Dec 25, 2016

this issue still isn't fixed.

1. \bin: http://regexr.com/3euta
2. \bin\b: http://regexr.com/3eutm
3. (^|\s)in($|\s): http://regexr.com/3eutg

  1. matches words like navbar-inverse-collapse and inverse-class
  2. matches words like navbar-in-example
  3. checks if string begins in or there is a space before in, and ensures that either string ends with in or a space comes after.

edit: cc @thednp @lcannon

@thednp
Copy link
Owner

thednp commented Dec 26, 2016

I think we will have a new hasClass(element,className) utility to work with a proper regex.

@thewisenerd
Copy link

@thednp cool!

at this point i have to manually patch javascript because i have a div with class name navbar-inverse-collapse :P

hoping this gets patched soon.

@dgrammatiko
Copy link

Why not use classList.contains('class-name') and polyfill ie?

@thednp
Copy link
Owner

thednp commented Dec 26, 2016

To keep polyfill dependency to as low as possible. I noticed some users haven't even heard of them.

@dgrammatiko
Copy link

@thednp according to Microsoft: https://www.microsoft.com/en-us/WindowsForBusiness/End-of-IE-support
EOL is in 3 weeks for IE 9, 10.
I guess people could either include ft polyfill service: https://polyfill.io/v2/docs/
or your version pumped with classList.
For me is better to have a clean ES5 code (and polyfill whatever missing) than try to make it "one fits all". But that's my 2c...

@RyanZim
Copy link
Contributor

RyanZim commented Dec 26, 2016

I think we're all looking forward to bootstrap v4; then we won't have to support IE8.

@dgrammatiko
Copy link

dgrammatiko commented Dec 26, 2016

@RyanZim BS v4 just dropped IE 9: twbs/bootstrap#21387

@thednp
Copy link
Owner

thednp commented Dec 26, 2016

yes, our v4 version will be IE9+ of course.

@RyanZim
Copy link
Contributor

RyanZim commented Dec 26, 2016

@DGT41 Thanks for pointing that out. If bootstrap is dropping IE 8 & 9, we should too IMO @thednp.

@thednp
Copy link
Owner

thednp commented Dec 26, 2016

@RyanZim we only need a proper Event.prototype + dispatcher polyfill for IE9+, so why just not, the same polyfill is required for IE10+ and I believe also for Ms Edge. (for future BSN4 I mean)

@RyanZim
Copy link
Contributor

RyanZim commented Dec 26, 2016

@thednp BS v4's css will be totally broken in IE9, why support IE9 for the JS?

@thednp
Copy link
Owner

thednp commented Dec 26, 2016

https://v4-alpha.getbootstrap.com/migration/

Dropped IE8 and iOS 6 support. v4 is now only IE9+ and iOS 7+. For sites needing either of those, use v3.
Added official support for Android v5.0 Lollipop’s Browser and WebView. Earlier versions of the Android Browser and WebView remain only unofficially supported.

But we are talking about the alpha, we don't know what's on the table with the RC/stable version.

@RyanZim
Copy link
Contributor

RyanZim commented Dec 26, 2016

Hosted docs are out of date with the alpha development branch. Actual latest docs are here: https://github.com/twbs/bootstrap/blob/v4-dev/docs/migration.md#browser-support.

Dropped IE8, IE9, and iOS 6 support. v4 is now only IE10+ and iOS 7+. For sites needing either of those, use v3.

@thednp
Copy link
Owner

thednp commented Dec 26, 2016

Ah great, so people will have to fork or look for other frameworks ;)

Off topic: I have had some ideas on hot to make a better HTML5 framework with plain JS some grids and few more components.

@dgrammatiko
Copy link

If you don't mind I will create some custom components based on your code (@thednp )

@thednp
Copy link
Owner

thednp commented Dec 26, 2016

Why should I? As long as the MIT license is all the way, we're all happy :)

@dgrammatiko
Copy link

Off topic: I have had some ideas on hot to make a better HTML5 framework with plain JS

@thednp there is https://frend.co but doesn't fulfilled most cases...

@thednp
Copy link
Owner

thednp commented Dec 26, 2016

Indeed, that's a bit too "one purpose" to me.

In my mind I think of a good typography + an improved&simplified bsn + some grid + some content light design.

@thednp
Copy link
Owner

thednp commented Jan 4, 2017

I was thinking about it and I wish to have a final note in regards to @RyanZim & @DGT41 remarks: our bsn here is for BS3 and will continue to support older browsers, just as the original script. The future version BSN will be also like BS4 for browser support.

@thewisenerd I started implementing a hasClass(element,classNAME) utility in all scripts, I'll commit the new code later today.

Happy New Year!!

@dgrammatiko
Copy link

Happy new year @thednp 🎉

Reasonable decision, I like it!

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

5 participants