Redo Tooltip Migration in `client/web`
Created by: lrhacker
Redoing the work that was reverted in #38866 and resolving the underlying problem.
Specifically, any <ButtonLink>
that is wrapped with a <Tooltip>
but does not have a to
prop (i.e. no href
for the link) will need to be wrapped with an additional element first (e.g. a <span>
).
The onClick
handler specified in <ButtonLink>
to prevent the default behavior (page reload) for an empty href
does not get properly composed with the rest of the tooltip props (I believe since it is defined within the underlying component, it is not a prop "for" the component). This results in that onClick being completely ignored (and the page will, in fact, reload when clicking the link).
I'm also wondering why, in these cases, we wouldn't want to just use a <Button>
element without specifying as={AnchorLink}
when we know it will not be a link to anything (since there is no to
property)? It might be worthwhile to make a separate ticket to investigate. Maybe just these specific toggle buttons should be switched, since they are buttons to toggle behavior on/off, and obviously are not links.
cc @umpox this might also be a semantic/accessibility thing, having <a>
tags that are not actually links to anything. From what I've read in the past, it seems like that is frowned upon - links should take you someplace else, and buttons should perform an action. Thoughts on that?
Test plan
Tooltips and buttons, including those to toggle file history and line wrapping, should all be working now.
App preview:
Check out the client app preview documentation to learn more.