Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APP-2206: break apart tooltip component for custom usage #384

Merged
merged 12 commits into from
Sep 29, 2023

Conversation

mcous
Copy link
Member

@mcous mcous commented Sep 22, 2023

Overview

Following up on #381, as I went to integrate the updated Tooltip, I saw that several Svelte usage sites of this tooltip would benefit from the "separate target from described element" work I mentioned in that ticket.

This PR breaks the <Tooltip> into its constituent parts:

  • <TooltipContainer> - a stateful wrapper
  • <TooltipTarget> - a wrapper that attaches mouse/focus event listeners
  • <TooltipText> - a presentational tooltip text component

You can continue to use <Tooltip>, or if that doesn't work for the UI in question, assemble the three parts however you need. In practice, I think this will mostly be "the visual target is different that the element on which we need to set aria-describedby.

I also upgraded the style calculation logic to automatically adjust scrolls/resizes/moves via floating-ui's autoUpdate.

2023-09-25 14 30 13

Change log

  • Add TooltipContainer, TooltipTarget, and TooltipText components that can be composed to create custom tooltips
  • Wire in autoUpdate so the recalculateStyle component export isn't needed

Review requests

I have updated the playground and the Storybook, which are good places to start with this PR. The actual positioning logic is hard to test without Playwright, but if you do see anything that could use unit testing that isn't currently tested, please call it out!

The best way to test the autoUpdate functionality:

  • Test the <SliderInput>
  • Scroll up and down on the dev playground, where I've added a permanently visible tooltip

@mcous mcous marked this pull request as ready for review September 25, 2023 17:00
Comment on lines -119 to -120
* TODO: Determine why this is being interpreted as an `any` type by the
* linter when it is of `() => Promise<void>`.
Copy link
Member Author

@mcous mcous Sep 25, 2023

Choose a reason for hiding this comment

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

This appears to be a limitation of the Svelte ESLint plugin; it's a little buggy when it comes to correctly manipulating the parse tree when TS is involved (e.g. sveltejs/eslint-plugin-svelte#583).

I've been moving type exports to separate pure TS files to work around the issue. It's a little irritating to move the interfaces out of the component that uses them; but it's worth it to avoid losing type safety

# Conflicts:
#	packages/core/src/lib/tooltip/tooltip.svelte
@mcous mcous requested a review from ehhong September 28, 2023 14:12
Copy link
Member

@DTCurrie DTCurrie left a comment

Choose a reason for hiding this comment

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

@mcous
Copy link
Member Author

mcous commented Sep 29, 2023

Once last fix caught by @ehhong: if state="visible", the tooltip would flash in the top left of its container before moving into position. Resolved by ensuring that invisible is applied if the position hasn't yet been calculated. I think this might have been a regression introduced by my recent work, but now its fixed and there's a test in place

@mcous mcous merged commit 4f11f7c into viamrobotics:main Sep 29, 2023
5 checks passed
@mcous mcous deleted the APP-2206_more-tooltip-tweaks branch September 29, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants