fixed a bug in Dropdown which caused an Error, except you are using j… #57
Conversation
…Query. Especially the function getClosestAncestor(). See issue #43
| /** | ||
| * Get closest ancestor that satisfies the condition | ||
| * @param {Element} el Element to find ancestors on | ||
| * @param {Function} condition Function that given an ancestor element returns true or false | ||
| * @returns {Element} Return closest ancestor or null if none satisfies the condition | ||
| */ | ||
| const getClosestAncestor = function(el, condition) { | ||
| let ancestor = el.parentNode; | ||
| while (ancestor !== null && !$(ancestor).is(document)) { | ||
| if (condition(ancestor)) { | ||
| return ancestor; | ||
| } | ||
| ancestor = ancestor.parentNode; | ||
| } | ||
| return null; | ||
| }; | ||
|
|
There was a problem hiding this comment.
is this PR only moves getClosestAnchor from global to dropdown? And deletes elementOrParentIsFixed? I don't see any other differences here... And I'm not sure how this fixes the linked issue...?
There was a problem hiding this comment.
Yes exactly!
getClosestAnchor
Because the function uses the $ operator, which is not accessible in the global.js File => This caused the Error.
So I searched where the function is used and it is only used by dropdown.js so I moved it there. This solves the Bug.
elementOrParentIsFixed
I saw that this function also uses the $ operatorm so I also search where the function is called. And it is never beeing called.
So I removed the function.
You can test it via the Select-Dropdown. Best regards and thanks for your time bro!
There was a problem hiding this comment.
So elementOrParentIsFixed was never documented and was some internal leftover?
There was a problem hiding this comment.
yes @DanielRuf I dont know why this function is there. I searched the whole repo and it wasnt used anywhere, so I removed it.
DanielRuf
left a comment
DanielRuf
left a comment
There was a problem hiding this comment.
I think we should mark it as internal by prepending _ to the function name.
| * @param {Function} condition Function that given an ancestor element returns true or false | ||
| * @returns {Element} Return closest ancestor or null if none satisfies the condition | ||
| */ | ||
| const getClosestAncestor = function(el, condition) { |
There was a problem hiding this comment.
Wait, is this a function in a function? Why not use _placeDropdown then?
There was a problem hiding this comment.
Yes, because it is used inside the function only... I dont know how to change the code in an existing PR. Can this be done via the WebInterface?
There was a problem hiding this comment.
You can push additional commits on the same branch in your repo or use https://github.com/materializecss/materialize/pull/57/files
|
Bump =) What is the current state of this PR, also regarding the review comments? |
|
Let's merge this =) |
|
You should have received an invitation to join @materializecss as member and collaborator for https://materializecss/materialize. |
3 participants
…Query. Especially the function getClosestAncestor(). See issue #43
Proposed changes
Fixes a Bug for operation. For more info see the issue.
Screenshots (if appropriate) or codepen:
Types of changes
Checklist: