-
Notifications
You must be signed in to change notification settings - Fork 188
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
[ARGG-1157][BpkTooltip]: Migrate tooltip to floating-ui #3485
base: main
Are you sure you want to change the base?
Conversation
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Browser supportIf this is a visual change, make sure you've tested it in multiple browsers. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
1 similar comment
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
dbb5c17
to
35587fb
Compare
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
35587fb
to
1b0b280
Compare
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
1b0b280
to
6ec9e40
Compare
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
*/ | ||
ariaLabel: string; | ||
/** | ||
* "target" should be a DOM element with a "ref" attached to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that still the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3485 to see this build running in a browser. |
Migrating the underlying library of the
BpkTooltip
to use the maintainers replacement libraryfloating-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:
TooltipPortal
, as we no longer need to use our custom React Portal implementation as this is handled via floating-uirenderTarget
prop as this is longer needed to apply where in the DOM the tooltip should be and is located byfloating-ui
relative to the targetportalStyle
andportalClassName
as these are no longer required as we aren't using a custom portalpopperModifiers
property as these no longer have an effect on the componentclassName
property to prevent overriding of componentMigration 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
andclassName
.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:
[KOA-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md