USWDS - Tooltip: Allow for dynamic changes #6431
Conversation
packages/usa-tooltip/src/index.js
Outdated
| tooltipTrigger.setAttribute("data-position", position); | ||
| } | ||
| // If the tooltip has already been instantiated, don't create another instance | ||
| const tooltipExists = () => { |
There was a problem hiding this comment.
Thanks for addressing this tooltip issue. I like how you've separated the logic between creating and updating tooltips.
A few suggestions to make the code more maintainable:
- Consider moving the
tooltipExistsandnewTooltipfunctions outside ofsetUpAttributesto improve readability - The function name
tooltipExistsis a bit confusing since it actually creates a new tooltip - perhapscreateTooltipwould be more accurate? - Similarly,
newTooltipcould be renamed toupdateTooltipto better reflect its purpose?
There was a problem hiding this comment.
My apologies for the delayed response, but you are absolutely right about the function names! It's one of those things that made sense during development for me, but when I step back it's definitely confusing.
I'm going to push back a little on the readability for moving each of the functions out of setUpAttributes, since the other two functions set up the tooltip attributes. I cannot, however, think of a better name for the setUpAttributes function that wouldn't be confusing elsewhere, so consider this note my pushback.
packages/usa-tooltip/src/index.js
Outdated
| // Identify the existing span element that wraps the trigger | ||
| const wrapper = tooltipTrigger.parentNode; | ||
| // Identify the existing span element that contains the tooltip | ||
| const tooltipBody = tooltipTrigger.nextElementSibling; |
There was a problem hiding this comment.
Using nextElementSibling to find the tooltip body is potentially fragile if the DOM structure changes. Consider using wrapper.querySelector(.${TOOLTIP_BODY_CLASS}) instead, which would be more resilient to structural changes.
packages/usa-tooltip/src/index.js
Outdated
| return { tooltipBody, position, tooltipContent, wrapper }; | ||
| }; | ||
|
|
||
| if (!tooltipTrigger.classList.contains(TOOLTIP_TRIGGER_CLASS)) { |
There was a problem hiding this comment.
The current approach checks if the trigger has the TOOLTIP_TRIGGER_CLASS class, which works for the initial implementation but may have edge cases.
When a tooltip is dynamically updated, the key check is:
if (!tooltipTrigger.classList.contains(TOOLTIP_TRIGGER_CLASS)) {
tooltipExists(); // Create new tooltip
} else {
newTooltip(); // Update existing tooltip
}This relies on the TOOLTIP_TRIGGER_CLASS already being present to determine if it's an update scenario. A more reliable approach would be to check the actual DOM structure - The proper tooltip structure consists of a parent wrapper (.usa-tooltip) containing both the trigger element and the tooltip body:
const isTooltipSetup = tooltipTrigger.parentNode &&
tooltipTrigger.parentNode.classList.contains(TOOLTIP_CLASS) &&
tooltipTrigger.parentNode.querySelector(`.${TOOLTIP_BODY_CLASS}`);
if (!isTooltipSetup) {
// Create new tooltip
} else {
// Update existing tooltip
}This would check if this trigger already properly wrapped in a tooltip structure.
There was a problem hiding this comment.
Thank you - I didn't have the time to get quite this complex while I was working on this, so I appreciate your thorough review!
|
Hey @sarahwylie , Thank you once again for working on this fix. I was playing around with the solution using the comments I suggested above by extracting tooltip creation and update logic into separate functions for better maintainability and also created a storybook test. You can find my implementation in the If you would like to check it out:
Feel free to incorporate any parts that work well for your implementation. Great work on the fix. |
Summary
Updated tooltip generation so devs can change tooltip text dynamically.
Breaking change
This is not a breaking change
Related issue
Resolves #6426
Preview link
Preview link:
https://github.com/user-attachments/assets/9d3cc8ab-a7e3-4072-9442-85056d946494
Problem statement
I applied the USWDS tooltip to a button component whose state changes based on the surrounding elements. When the state of the button changes, the tooltip text no longer conveys the appropriate meaning. When trying to dynamically update the text, a new
<span>element was created each time the state of the text was updated, which quickly spiraled the element out of control.Solution
This code checks whether a tooltip has already been initiated on the target element before implementing the next steps. If the trigger has not yet been applied to the element, then the tooltip will proceed through the full setup steps. If the trigger already exists, then the code will check if the title has been changed, if the position has been changed, and if any additional classes have been added to the element, then makes only the necessary updates.
Testing and review
I made this change as a patch for a project, and I wanted to contribute the work back in case it is useful for others.
Dependency updates
N/A