Skip to content

Add a for attribute to tooltip#116

Open
treeder wants to merge 1 commit into
mainfrom
for-attribute-tooltip
Open

Add a for attribute to tooltip#116
treeder wants to merge 1 commit into
mainfrom
for-attribute-tooltip

Conversation

@treeder
Copy link
Copy Markdown
Member

@treeder treeder commented May 12, 2026

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a "for" property to the Tooltip component, allowing it to target and anchor to external elements. The implementation includes logic for finding the target element, managing event listeners, and calculating fixed positioning. Feedback focuses on improving robustness: addressing a potential memory leak when attaching listeners asynchronously, ensuring reactivity when the "for" property changes dynamically, and handling window resize events to maintain correct alignment.

Comment thread tooltip/tooltip.js
this.targetElement = this

// Defer finding the external target to ensure it is rendered
requestAnimationFrame(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using requestAnimationFrame to defer event listener attachment can lead to a memory leak. If the component is disconnected before the callback executes, listeners will be attached to the targetElement (which may be an external element) but will never be removed because disconnectedCallback has already run. Adding a check for this.isConnected ensures listeners are only attached if the component is still active.

    requestAnimationFrame(() => {
      if (!this.isConnected) return

Comment thread tooltip/tooltip.js
headline: { type: String },
text: { type: String },
open: { type: Boolean, reflect: true },
for: { type: String },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The for property logic is currently only executed once during connectedCallback. If the for attribute is changed dynamically, the tooltip will not update its target or listeners. Additionally, if for is removed, the internal tooltip element will retain the position: fixed style applied by updatePosition. Consider handling these changes reactively in the updated lifecycle method to ensure the component stays in sync with its properties.

Comment thread tooltip/tooltip.js
}
}

updatePosition() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The tooltip position is only calculated when it is first opened. If the window is resized or the layout shifts while the tooltip is visible, it will become misaligned with the target element. Consider adding a resize event listener to the window while the tooltip is open to trigger updatePosition and keep it correctly anchored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant