-
-
Notifications
You must be signed in to change notification settings - Fork 78.8k
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
Fix getSelectorFromElement when # is a selector #21615
Conversation
See this PR #21654 that handle the problem differently and cover more (all) possible cases of selector. |
Yes I forget one case but I'll update my PR thank you @vanduynslagerp |
I didn't change how the core of Bootstrap works but I added what you did in your PR by removing the regex because as you said it fix #18641 too 👍 |
Really sorry to insist...but here is a few remarks. Only the dropdown and the alert component expect to Also consider other possible wrong value such Finally you call Jquery on the value For all these reasons and several other I decided to rewrite the function The point I was trying to make is that the best way to handle this is to get rid of the regex and use the try catch. This way we can convert all cases and we don't have to add more The point of my PR was in the first place to fix #18641 and improve the mechanism by making more robust. I realize later on after posting the PR, that it would also fixe #21328 so I mentioned it here. |
Yes you're right on this :
So I'll update a bit my PR thank you 👍 In your PR the same problem is present because if the selector is set to const target = targets ? targets[0] : null But if targets is equal to |
My point was that after I made my PR (without seeing yours before, sorry...) I realized that it also fixes the same problem and covers more ground. There is no point in having both #21654 and this one. Anyway I don't want to insist more.
Yes that's on purpose. If there is no matching element that can be used as target then the variable |
I add on my last commit, a check if the selector exist in the DOM or not and with that we don't have to edit all plugins because it works as you described. Your dropdown unit test is very usefull 👍 |
The more modifications you make, the more your PR is looking like mine. |
I finished my PR I wont add anything. My PR only avoid the issue when ``#`` is used in selector that's all
…On 11 Jan 2017, 18:33, at 18:33, Pierre-Denis Vanduynslager ***@***.***> wrote:
The more modifications you make, the more your PR is looking like mine.
I spent a day working on #21654 and going through all this cases,
details and test. So once you will go through all of them, I imagine
the code will be similar to mine.
So I'm not really what we are doing here....
I just wanted to make the point, in my first comment, that I believe
#21654 solves #21328 and other reported and non-reported issues in a
more thorough way.
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#21615 (comment)
|
Lemme know if we still need to do #22128 as well. |
Check if
data-target
contain#
to avoid JQuery errorClose : #21328