-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
…hanging this test passes in Chrome Canary v54
part of this was to make the slot name change unregister / reregister. Another part was to make the slotNodeFromSlot function work under a closed shadow root, as assignedSlot would return null in this case. We still need to fix the function to work with setAttribute
…d nodes as children
@@ -168,7 +168,7 @@ function slotNodeIntoSlot(slot, node, insertBefore) { | |||
} | |||
|
|||
function slotNodeFromSlot(node) { | |||
const slot = node.assignedSlot; | |||
const slot = nodeToSlotMap.get(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure we can use get the slot when in closed
mode
const slotHasRoot = root; | ||
|
||
if (slotHasRoot) { | ||
removeSlotFromRoot(root, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks weird. Why are you doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because the previous DOM nodes must be unassigned. There might be a better way to do this, like unassignNodesFromSlot()
. I'm thinking this might be able to be used elsewhere, too.
That said, I'd get this working and tests green before trying to refactor that now.
problem was that you can't put undefined into a weakmap
lGtm The capital G is for "GREAT". Also, to test the case sensitivity of gh |
lgtm |
1 similar comment
lgtm |
@@ -54,7 +54,7 @@ describe('slot/distribution', () => { | |||
expect(frag.childNodes.length).to.equal(0); | |||
}); | |||
|
|||
describe('changing slot name with three children', () => { | |||
describe('changing slot name with more than two', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more than two what? :D
no lgtm from me until you fix airbnb linting ;p |
lgtm |
worked on this with @lukebatchelor