Rewritten tab without jquery#24094
Conversation
There was a problem hiding this comment.
Use Util.getUID (https://github.com/twbs/bootstrap/blob/v4-without-jquery/js/src/util.js#L51) instead of that
There was a problem hiding this comment.
why did you changed that selector ?
There was a problem hiding this comment.
> li > .active is not a valid selector. Using with querySelector/querySelectorAll throws a SyntaxError.
There was a problem hiding this comment.
yep so you should find li children and after that .active elements, no need here to implement :scope
There was a problem hiding this comment.
Sorry, can't understand. Are you suggesting not to use a selector? Or a double use of .children? In both cases it could be very slow
There was a problem hiding this comment.
Nope you're right that's better like that, sorry I didn't know :scope before your PR 😄
There was a problem hiding this comment.
:scope deprecated?) https://developer.mozilla.org/en-US/docs/Web/CSS/:scope
There was a problem hiding this comment.
Nothing indicates that :scope selector has been deprecated. It is part of the CSS Selectors Level 4 Working Draft. Some browsers have removed the scoped style tag attribute, but the :scope pseudo-class is still supported and works correctly in CSS (works the same as :root) and in querySelector/querySelectorAll
alekitto
force-pushed
the
tab-without-jquery
branch
from
52c47b6 to
bbdf3c3
Compare
There was a problem hiding this comment.
you should merge those two if (with the other one above this one)
There was a problem hiding this comment.
this polyfill come from MDN, why did you change that ?
There was a problem hiding this comment.
Tests are failing with that if. In any case, the DOM Standard specification (https://dom.spec.whatwg.org/#dom-element-closest) doesn't require the element to be a discendant of the document object
There was a problem hiding this comment.
that's right but I don't see a use case where the element should be outside of the document.documentElement 🤔
There was a problem hiding this comment.
Probably some tests don't add elements to the DOM before execution. If you prefer, I can revert this change and fix those tests.
There was a problem hiding this comment.
Not a big fan of that, because if it's an ID you'll use querySelector instead of getElementById which is faster
There was a problem hiding this comment.
Multiple selector starting with # (ex: #id > li, #id1 > .active) will match and return a wrong result
There was a problem hiding this comment.
However, I think this is in the wrong PR, i've encountered an error on this while working on ScrollSpy 😅
There was a problem hiding this comment.
@Johann-S Should I revert this? Or fix testing the selector against a regex that excludes complex selectors?
There was a problem hiding this comment.
Nope finally I think it's fine like this 👍
alekitto
force-pushed
the
tab-without-jquery
branch
from
bbdf3c3 to
b902331
Compare
Johann-S
force-pushed
the
v4-without-jquery
branch
from
76ee411 to
38bebc9
Compare
alekitto
force-pushed
the
tab-without-jquery
branch
2 times, most recently
from
1253847 to
ff0ef42
Compare
Johann-S
force-pushed
the
v4-without-jquery
branch
2 times, most recently
from
e692aa3 to
ab6c322
Compare
alekitto
force-pushed
the
tab-without-jquery
branch
from
ff0ef42 to
96f7fc6
Compare
Johann-S
force-pushed
the
v4-without-jquery
branch
from
e310e55 to
d252d27
Compare
|
@alekitto can you update your branch please ? 👍 |
alekitto
force-pushed
the
tab-without-jquery
branch
from
96f7fc6 to
49cbbd4
Compare
|
@Johann-S done 👍 |
There was a problem hiding this comment.
It seems an unrelated change here 🤔 Maybe can you explain it ?
There was a problem hiding this comment.
This check was introduced to fix fading when no initial panel was active, but i've probably missed a test. I'll push it asap, but if you want to keep things separated, i can revert this check and submit a pr just for this case.
There was a problem hiding this comment.
Is it an issue in our current v4-dev branch ?
There was a problem hiding this comment.
I’ve just tested it on the v4-dev, the issue is present in the visual test page
There was a problem hiding this comment.
So I prefer if you can create a PR (which target v4-dev) to resolve that or if you can create an issue which describe that
There was a problem hiding this comment.
Correct. I've pushed a PR to explain and resolve the issue on v4-dev branch
There was a problem hiding this comment.
I've reverted this change for this PR, as it is not related
alekitto
force-pushed
the
tab-without-jquery
branch
from
49cbbd4 to
b705cb5
Compare
3 participants
Related to #23586