[ARGG-1157][BpkTooltip]: Migrate tooltip to floating-ui #3485

olliecurtis · 2024-06-03T15:19:09Z

Migrating the underlying library of theBpkTooltip to use the maintainers replacement library floating-ui, a much more lightweight library and has a lot of functionality built in that means we can remove our use of custom built react Portals!

This is one PR of a few to update components away from popperjs to floating-ui so can be used as a base!

Changes here are as follows:

Removed TooltipPortal, as we no longer need to use our custom React Portal implementation as this is handled via floating-ui
Removed the renderTarget prop as this is longer needed to apply where in the DOM the tooltip should be and is located by floating-ui relative to the target
Removed the portalStyle and portalClassName as these are no longer required as we aren't using a custom portal
Removed the popperModifiers property as these no longer have an effect on the component
Removed className property to prevent overriding of component
Converted component to Typescript and generated types
Removed snapshot testing as this is unreliable and not best practice
Added visual testing

Migration guide:

The main changes for this component is removing properties that have minor to no impact to the current functionality of the component.

So remove use of the following properties: renderTarget, portalStyle, portalClassName, popperModifiers and className.

Should the design/style not be fit for purpose please align with design to make sure they use the component as expected before getting in touch.

Remember to include the following changes:

Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
README.md (If you have created a new component)
Component README.md
Tests
Storybook examples created/updated
For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

github-actions · 2024-06-03T15:22:48Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-03T16:15:35Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-03T16:17:17Z

	Warnings
⚠️	Package source files (e.g. `packages/package-name/src/Component.js`) were updated, but snapshots weren't. Have you checked that the tests still pass?
⚠️	Package source files (e.g. `packages/package-name/src/Component.tsx`) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against e1b9683

github-actions · 2024-06-03T16:40:05Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-04T09:06:12Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-04T09:19:12Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-04T12:57:19Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-04T14:38:27Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-04T17:19:11Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-17T10:33:43Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-19T08:31:04Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

anambl · 2024-06-24T17:33:31Z

packages/bpk-component-tooltip/src/BpkTooltip.tsx

+   */
+  ariaLabel: string;
+  /**
+   * "target" should be a DOM element with a "ref" attached to it.


is that still the case?

Yes this is still the case, we will still want people to pass through a ReactElement and not a function like document.getElementById which is what the popover did, but the tooltip was never built to support that only

<div> My target </div>

I mean the ref part with a "ref" attached to it. . I see we're using floating ui to set the ref so I don't think the consumer has to? https://github.com/Skyscanner/backpack/pull/3485/files#diff-2702cb346943a8761bb30c82e1a61efeae40297ed8cee8028da4376b719dc73cR132-R138

Also interesting that you can clone an element and attach a ref to it 😮 I think that's new-ish, I think a few years ago it was not possible

Ah gotcha! Let me do a test with removing that target ref and see what magic happens!

Also interesting that you can clone an element and attach a ref to it 😮 I think that's new-ish, I think a few years ago it was not possible

Interesting didn't realise it was a newer feature! I guess it is just a property to a DOM element so is easy to add this prop as any other?

Ah gotcha! Let me do a test with removing that target ref and see what magic happens!

Cool, have you checked that?

Interesting didn't realise it was a newer feature! I guess it is just a property to a DOM element so is easy to add this prop as any other?

Yeah, I remember it wasn't possible before, but not sure why 🤔

Had a check and yes ref can be removed so will update the PR now

github-actions · 2024-06-26T08:11:27Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-27T14:59:15Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-06-27T15:49:47Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-07-16T15:08:53Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-11-20T16:25:33Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

github-actions · 2024-12-04T10:49:27Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

anambl · 2025-02-10T11:28:17Z

@olliecurtis Are we still doing this, or can we close it?

olliecurtis · 2025-02-10T11:29:38Z

@olliecurtis Are we still doing this, or can we close it?

Yep we still want to be doing this and its still a valid PR just needs review 😄 as popperjs is deprecated and no longer supported

c.c. @anambl

skyscanner-backpack-bot · 2025-02-10T11:35:25Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

anambl · 2025-02-10T11:36:40Z

@olliecurtis Are we still doing this, or can we close it?

Yep we still want to be doing this and its still a valid PR just needs review 😄 as popperjs is deprecated and no longer supported

c.c. @anambl

Awesome! There was a conversation here and a question from me - not sure if you've had a look at that? The reason I asked it is because I remember adding those changes and making the ref required, but not sure if it's still needed with the new library.

Also as we now have a release schedule in place, we won't be able to just merge as is. I think that given this PR was created before we implemented the release schedule, we have 2 options:

if there aren't too many usages of it, we could merge it in March directly.
if there are, or we aren't sure, we will need to make it backwards compatible.
What do you think?

olliecurtis · 2025-02-10T11:46:57Z

Will check out the comment and remind myself on whats going on there to respond to you!

Also as we now have a release schedule in place, we won't be able to just merge as is. I think that given this PR was created before we implemented the release schedule, we have 2 options:

if there aren't too many usages of it, we could merge it in March directly.

if there are, or we aren't sure, we will need to make it backwards compatible.

What do you think?

I can check the numbers but from my memory this should work with backwards compatibility and not require too much rework from a consumer, as this was done to manage the impact the same as the BpkPopover when we did the same migration as we didn't want to interfer with the flow but also need to migrate from this unsupported library as it introduces a security risk now popperjs isn't supported :)

skyscanner-backpack-bot · 2025-02-18T10:10:31Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

skyscanner-backpack-bot · 2025-02-19T16:09:23Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

skyscanner-backpack-bot · 2025-04-08T08:43:55Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

olliecurtis · 2025-04-08T08:58:22Z

Also as we now have a release schedule in place, we won't be able to just merge as is. I think that given this PR was created before we implemented the release schedule, we have 2 options:

if there aren't too many usages of it, we could merge it in March directly.

if there are, or we aren't sure, we will need to make it backwards compatible.

What do you think?

@anambl I did checking on the changes in my PR and how consumers use it and the impact to this is that it still works as previous :) as the changes here are removing the BpkTooltipPortal and what I did was carry the props across to the BpkTooltip main file as they are now collapsed and still provide critical functionality

… in tests

skyscanner-backpack-bot · 2025-04-08T09:22:00Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

skyscanner-backpack-bot · 2025-04-08T09:32:01Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

examples/bpk-component-tooltip/examples.tsx

examples/bpk-component-tooltip/examples.js

skyscanner-backpack-bot · 2025-04-08T22:02:23Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

GCpigsic

Work well in my local, LGTM

GCpigsic · 2025-04-09T04:19:10Z

packages/bpk-component-tooltip/src/BpkTooltip.tsx

+    role: 'tooltip',
+    ref: refs.setReference,
+    ...getReferenceProps(),
+  });


works well 👍

GCpigsic · 2025-04-09T06:25:03Z

packages/bpk-component-tooltip/src/BpkTooltip.tsx

+                  ref={arrowRef}
+                  context={context}
+                  id={ARROW_ID}
+                  className={arrowClassNames}


I just thought we could use the width, height, and fill of the FloatingArrow directly so that we don't need arrowClassNames.
https://floating-ui.com/docs/floatingarrow#height
but unfortunately, in backpack-foundations, it doesn't export spacing token (sm, md,xl ...) .
So i think using arrowClassNames is ok

I tried using these directly but they had issues with actually styling the arrow and broke the styling of it, so this approach was chosen to ensure the arrow could match the style and colour of the box :)

gert-janvercauteren · 2025-04-09T06:38:17Z

Hi @olliecurtis late to the party :(
Can we make the tooltip content copyable? As in, you can move the mouse pointer to the tooltip content, and it will stay so you can select text.

It's one of the things reported (multiple times) on the recent Accessibility audit :D

It seems that floating UI does support this, is this simple enough to add to our implementation?
https://floating-ui.com/docs/react-examples

Copilot

Copilot reviewed 9 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (3)

packages/bpk-component-tooltip/src/BpkTooltip.module.scss: Language not supported
packages/bpk-component-tooltip/src/snapshots/BpkTooltip-test.tsx.snap: Language not supported
packages/bpk-component-tooltip/src/snapshots/BpkTooltipPortal-test.tsx.snap: Language not supported

Copilot · 2025-04-09T06:39:03Z

packages/bpk-component-tooltip/src/BpkTooltip.tsx

+    role,
+  ]);
+
+  const targetWithAccessibilityProps = cloneElement(target, {


The target element is being assigned a role of 'tooltip', but according to ARIA guidelines the tooltip container (not the target) should have role 'tooltip'. Consider removing the role from the target and instead use aria-describedby on the target to reference the tooltip content.

olliecurtis · 2025-04-09T08:26:34Z

Hi @olliecurtis late to the party :( Can we make the tooltip content copyable? As in, you can move the mouse pointer to the tooltip content, and it will stay so you can select text.

It's one of the things reported (multiple times) on the recent Accessibility audit :D

It seems that floating UI does support this, is this simple enough to add to our implementation? https://floating-ui.com/docs/react-examples

Hey @gert-janvercauteren

I can take a look here! I guess for an interaction such as this, this is where we would advocate for using the BpkPopover instead of the tooltip if there is content inside that wishes to be copied of interacted with. :) What would be your thoughts for advocating for that in those scenarios?

If we decide to add this functionality, can we possibly defer that to a separate minor PR to add functionality so we don't block this going in during the current major cycle and its becomes a year (total time since open) before this is closed off? I can quickly spin that PR up once this one goes through so its not forgotten about and still goes in swiftly

Faye-Xiao · 2025-04-09T09:19:02Z

packages/bpk-component-tooltip/src/BpkTooltip.tsx

-}: TooltipProps) => {
-  const classNames = getClassName('bpk-tooltip', className);
+}: Props) => {
+  const [isOpenState, setIsOpenState] = useState(isOpen);


Not blocker:
Hi @olliecurtis, I noticed that you introduced the FloatingPortal and the isOpen property to manage its state. This made me think about the controlled mode of BpkTooltip and its related code.

In theory, BpkTooltip should not be used in situations where the parent component controls whether it is open or closed. Allowing this might lead to scenarios where the isOpen state does not change when the FloatingPortal unmounts in certain situations. Have we stated this somewhere? 🤔
I'm not quite sure if it is used in the right way by consumers. I have worked on an experimental feature named CE Budget filter which used a BpkPopover(similar implementation, we got that implementation from another squad due to a feature ownership change) and met this question. I know that's not the right way to use it. I feel like we need to highlight that to designers and engineers.

Faye-Xiao · 2025-04-10T06:22:15Z

Hi @gert-janvercauteren for this comment, can we make it to the later PR? This PR is more focused on the core code migration.

Faye-Xiao · 2025-04-10T06:34:46Z

Hi @gert-janvercauteren for this comment, can we make it to the later PR? This PR is more focused on the core code migration.

Made an offline chat. The functionality mentioned in Gert's comments is missing from the original component. That's not a blocker of this PR. That would be introduced in later PR.

skyscanner-backpack-bot · 2025-04-10T07:27:38Z

Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser.

olliecurtis added the major Breaking change label Jun 3, 2024

olliecurtis force-pushed the ARGG-1157-tooltip-to-floating-ui branch from dbb5c17 to 35587fb Compare June 4, 2024 09:15

olliecurtis force-pushed the ARGG-1157-tooltip-to-floating-ui branch from 35587fb to 1b0b280 Compare June 4, 2024 12:53

olliecurtis force-pushed the ARGG-1157-tooltip-to-floating-ui branch from 1b0b280 to 6ec9e40 Compare June 4, 2024 14:36

anambl reviewed Jun 24, 2024

View reviewed changes

olliecurtis requested a review from anambl July 15, 2024 14:45

olliecurtis added 8 commits April 8, 2025 09:38

Formatting and adding visual testing example

9586a92

Returned comment to props for documentation

4f82791

Fix type failures and fix unused prop

734a87f

Fixing a11y voiceover issues

1a61a8f

Testing adding timeout for stories to render

dd7b2ec

Fixing a11y voiceover issues and attaching to references in stories

1ac716e

[ARGG-1157]: Update tooltip to attach to body outside of target elements

7b04057

Remove the autoflip to avoid problems and learnings from the popover

a486065

olliecurtis force-pushed the ARGG-1157-tooltip-to-floating-ui branch from b93e93a to a486065 Compare April 8, 2025 08:38

PR feedback addressing: Remove comment about ref and fix act warnings…

10bb8e5

… in tests

GCpigsic reviewed Apr 8, 2025

View reviewed changes

examples/bpk-component-tooltip/examples.tsx Outdated Show resolved Hide resolved

examples/bpk-component-tooltip/examples.js Show resolved Hide resolved

Remove unused useRef import

38ac538

GCpigsic approved these changes Apr 9, 2025

View reviewed changes

GCpigsic reviewed Apr 9, 2025

View reviewed changes

gert-janvercauteren requested a review from Copilot April 9, 2025 06:38

Copilot AI reviewed Apr 9, 2025

View reviewed changes

Faye-Xiao reviewed Apr 9, 2025

View reviewed changes

Merge branch 'main' into ARGG-1157-tooltip-to-floating-ui

e1b9683

Faye-Xiao merged commit 53f06f8 into main Apr 10, 2025
9 checks passed

Faye-Xiao deleted the ARGG-1157-tooltip-to-floating-ui branch April 10, 2025 07:35

[ARGG-1157][BpkTooltip]: Migrate tooltip to floating-ui #3485

[ARGG-1157][BpkTooltip]: Migrate tooltip to floating-ui #3485

Conversation

olliecurtis commented Jun 3, 2024

github-actions bot commented Jun 3, 2024

github-actions bot commented Jun 3, 2024

github-actions bot commented Jun 3, 2024 • edited by skyscanner-backpack-bot bot Loading

Browser support

github-actions bot commented Jun 3, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 17, 2024

github-actions bot commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 26, 2024

github-actions bot commented Jun 27, 2024

github-actions bot commented Jun 27, 2024

github-actions bot commented Jul 16, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Dec 4, 2024

anambl commented Feb 10, 2025

olliecurtis commented Feb 10, 2025

skyscanner-backpack-bot bot commented Feb 10, 2025

anambl commented Feb 10, 2025

olliecurtis commented Feb 10, 2025

skyscanner-backpack-bot bot commented Feb 18, 2025

skyscanner-backpack-bot bot commented Feb 19, 2025

skyscanner-backpack-bot bot commented Apr 8, 2025

olliecurtis commented Apr 8, 2025

skyscanner-backpack-bot bot commented Apr 8, 2025

skyscanner-backpack-bot bot commented Apr 8, 2025

skyscanner-backpack-bot bot commented Apr 8, 2025

GCpigsic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gert-janvercauteren commented Apr 9, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Apr 9, 2025

Choose a reason for hiding this comment

olliecurtis commented Apr 9, 2025

Faye-Xiao Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

Faye-Xiao commented Apr 10, 2025

Faye-Xiao commented Apr 10, 2025

skyscanner-backpack-bot bot commented Apr 10, 2025

github-actions bot commented Jun 3, 2024 •

edited by skyscanner-backpack-bot bot

Loading

gert-janvercauteren commented Apr 9, 2025 •

edited

Loading

Faye-Xiao Apr 9, 2025 •

edited

Loading