fix: advanced marker anchoring #577
Conversation
There was a problem hiding this comment.
Looking really good!
What I would really like to have still is an overview of which elements are created, when and why, and how this all works together. Not sure if that has to be in the documentation or just somewhere in the code. We could also tackle this in a follow-up.
| @@ -148,8 +148,10 @@ describe('map and marker-library loaded', () => { | |||
| .get(google.maps.marker.AdvancedMarkerElement) | |||
| .at(0) as google.maps.marker.AdvancedMarkerElement; | |||
|
|
|||
| expect(marker.content?.firstChild).toHaveClass('classname-test'); | |||
| expect(marker.content?.firstChild).toHaveStyle('width: 200px'); | |||
| expect(marker.content?.firstChild?.firstChild).toHaveClass( | |||
There was a problem hiding this comment.
wondering if we should replace this with something like "expect that there is an element with classname-test somewhere in marker.content" – feels like specifying the exact hierarchy here isn't actually what we want to test.
There was a problem hiding this comment.
Yes, that sounds like a better approach.
src/components/advanced-marker.tsx
Outdated
| @@ -31,6 +31,10 @@ export function isAdvancedMarker( | |||
| ); | |||
| } | |||
|
|
|||
| function isElementNode(node: Node): node is HTMLElement { | |||
| return (node as Node).nodeType === Node.ELEMENT_NODE; | |||
There was a problem hiding this comment.
isn't as Node redundant here, since that's the type given above?
There was a problem hiding this comment.
Ah yes, you're right.
usefulthink
changed the title
Agreed, that would be helpful. |
First draft for improved and more robust advanced marker anchoring. Still needs some testing...
Main change is moving away from the "0, 0" div for the anchoring reset. That seemed to cause problems in a few situations
This should address and fix: