Replies: 25 comments 3 replies
-
Relating to this - one thing I've been slightly annoyed with in the current implementation is that when So going from But - since there is also the |
Beta Was this translation helpful? Give feedback.
-
So you'd want: const {refs} = useFloating();
return <>
<div ref={refs.setReference} />
<div ref={refs.setFloating} />
</>; The main use case where that doesn't make sense is when passing in virtual elements in an effect/event handler I guess? Since it doesn't attach itself to the useEffect(() => {
refs.setReference({ ... });
}, [refs]); Although since it attaches itself to |
Beta Was this translation helpful? Give feedback.
-
No. 2 and No. 4 are implemented in the latest versions, I just need to switch the docs to use the new API, but the original callback ref aliases will likely be kept around forever (or at worst, with a deprecation warning in the next major), since they add almost no cost, and I don't want to break anyone's code and make upgrading easy. For No. 3, if implemented, it wouldn't be an API like: const {refs} = useFloating({
// You can sync one or the other, allowing you to mix the internal ref
// setters with external elements
reference,
floating,
}); I'd also like to make the main |
Beta Was this translation helpful? Give feedback.
-
Welp, thanks for the callout, I have been lucky enough to start working on some of our "floating components" exactly at a time floating-ui was release, otherwise I would be one of those legacy Popper users 😅
Just a thought, but keeping them does keep the API more complex, and likely we'll still need the docs to tell what these are - otherwise someone looking at the return value of Also, if one would like to make the returned value from the hook accessible through context (like the official component examples also suggest)...
But would like to keep the shared context value lean, the extra props are sort of a nuisance? I guess one could do I'll try to write up some general thoughts now. For the most part, the library works great! One thing I have noticed is the general configuration complexity. I suppose this is sort of a necessity for a library that tries to be fully tree-shakeable, where every feature is strictly opt-in. Though, my experience so far is that even if a single component does not use all of the exports from floating-ui, likely a user of this lib will build several components based on it, so in the end most of the library will end up in the bundle anyway. I think the only hooks I don't have a usecase for yet is Just as an example, my Tooltip component imports and uses all of these from floating-ui:
A whopping 23 line import statement (minus 2 if you remove type imports). But in the component itself, all of these need to be configured one-by-one as well, so there's a lot of configuration. Some features need to be configured in multiple places or through multiple hooks, which is sort of weird, even though I understand the reasoning.
This one has three exports: Additionally, it needs to be integrated to both Since the delay group functionality only affects Also, for the integration in
This one has 4 required imports: There are several "nuisances" regarding this. One is the fact that basically every component that uses FloatingTree, needs to have that wrapper component implemented, which is sort of boilerplatey. I get why that is necessary - i.e. that the context provider needs to be a parent component of the one that actually uses the
Another, more of a docs issue for Third -
Would it be possible to, instead, use something like this?
I.e. let Stopping now, I hope I did not ramble too much here 😅 All in all, I think everything is great and - most importantly - it works. Just would like some of the features' configuration to be a bit simpler. |
Beta Was this translation helpful? Give feedback.
-
@mihkeleidast thank you for writing that up, it's very helpful! If you have any more thoughts, some kind of paginated document (Notion?) with each page being a separate API feedback would be great actually
I think they can have tsdoc notes for IntelliSense noting they are aliases for the
There are definitely a lot of imports, as you mentioned due to tree-shaking reasons. For example, The alternative is a prop-based API with minimal imports to use, but large bundle size. Maybe we can try grouping some parts together better somehow to reduce the number of imports needed. I'd ideally like to keep it as fairly tree-shakeable as possible like the main positioning packages, but there's a limit where it becomes too much (and I think the I think the main positioning library has a much better balance of prop-based config and separate modules.
Looking at this, it looks like an oversight and can indeed be made simpler, I'll look into it.
It's clear that there should just be more coordination of the main hook with less decoupling overall (and therefore configuration needed). As you mentioned, in practice many of the parts are used anyway, so separate tree-shakeable parts aren't that much help bundle size-wise. |
Beta Was this translation helpful? Give feedback.
-
Hmm, maybe it would be best if we collect the "ideas" here and then you can split stuff up to separate issues if something needs working on / discussed further? I think that would potentially encourage feedback from other users more, another tool like Notion will likely go unnoticed for most. Adding one more thing here now: Enabling/disabling the entire hookCurrently, in several components, I have either a For the interaction hooks, this needs to be specified to each subhook one by one:
While I think that the prop on the subhooks themselves is necessary to be kept (there can be more complex usecases where only a single hook can be disabled on some condition), for such "global disabling", it would be good if we could specify this only once to
Might be even beneficial for perf, if disabled, we can bail out of some of the work being done in the core hook? |
Beta Was this translation helpful? Give feedback.
-
Based on the docs:
|
Beta Was this translation helpful? Give feedback.
-
@atomiks for consideration: perhaps FloatingDelayGroup should be renamed to just FloatingGroup? We ran into an issue with small reference elements that have long tooltips that also have safePolygon closing enabled: https://codesandbox.io/s/sharp-chandrasekhar-18gn6s?file=/src/Tooltip.tsx This often results in a situation where both of the tooltips remain open, because of the safePolygon closing, which is sort of unfortunate result visually: This is especially concerning when moving from "b" to "a", as with the current portal root logic (related: #2094), the "b" tooltip will remain on top of "a" tooltip, even though it is rendered later interaction-wise. Ideally, in those situations, we'd like to say that there should only be a single Tooltip open at a time in that group. So I found that we can actually leverage FloatingDelayGroup for this: https://codesandbox.io/s/competent-golick-nfshxc?file=/src/App.tsx But |
Beta Was this translation helpful? Give feedback.
-
@mihkeleidast while that works, if you try to move the cursor into the tooltip by quickly traversing the other element, it ends up closing expectedly and showing the other tooltip even if that wasn't your intent. Maybe not the biggest deal in that situation, but for submenus it's more important. The lib's best solution for it right now is combining |
Beta Was this translation helpful? Give feedback.
-
Yup, I'm aware of the intent tradeoff, I think it's fine in that scenario, as the other solutions also have their own tradeoffs and this is likely the easiest to implement/maintain. But I don't really want to discuss the particulars of that too much in this issue - the idea is still that the grouping logic can be used for more things than just delay grouping, which is not evident from the current naming logic or documentation. What do you think about that? Should it only be used for delays, so the other uses are discouraged (i.e. they may break when updating), or is that a valid usecase, in which case it would be good to at least document it / rename the components? |
Beta Was this translation helpful? Give feedback.
-
I think renaming it without |
Beta Was this translation helpful? Give feedback.
-
I plan on creating a
|
Beta Was this translation helpful? Give feedback.
-
Bumping some of the other issues that might be considered breaking and as such, perhaps would be good to try to solve before v1? Namely:
I of course understand that it might not be feasible to push everything into v1, but maybe it would be good to understand if those issues can be solved with simple "feature updates" or if they will require breaking APIs, which would potentially need to be punted to faraway future release (assuming you don't want to do breaking changes too often). |
Beta Was this translation helpful? Give feedback.
-
IDK if this is ther right place, but i'd like to throw my hand up for wanted an easier time passing reference elements to the useFloating hook. As is most of our API's allow consumers to pass in an element as the reference so we have this mildly awkward |
Beta Was this translation helpful? Give feedback.
-
@jquense I think that's No. 3 in the list in the original post ("external element synchronization") which you should mix/match with the Possible APIs to synchronize with an external value: useFloating({
reference,
floating,
});
useFloating({
referenceElement,
floatingElement,
});
useFloating({
elements: {
reference,
floating,
}
}); |
Beta Was this translation helpful? Give feedback.
-
Hi, can anyone provide an example on how the external reference should look like? Given the note in the docs says:
|
Beta Was this translation helpful? Give feedback.
-
@smellai store the element in state (here it is with a type): const [referenceEl, setReferenceEl] = React.useState<HTMLElement | null>(null);
const {refs} = useFloating();
useLayoutEffect(() => {
refs.setReference(referenceEl);
}, [refs, referenceEl]);
return <div ref={setReferenceEl} /> Note: you will be able to do this in the next version, which will release within a couple of days: const [referenceEl, setReferenceEl] = React.useState<HTMLElement | null>(null);
// In the next version:
useFloating({
elements: {
reference: referenceEl,
}
});
return <div ref={setReferenceEl} /> In either case you should use state, not a ref, to make it reactive to updates. |
Beta Was this translation helpful? Give feedback.
-
@atomiks any update? 🙏 |
Beta Was this translation helpful? Give feedback.
-
@smellai update on what? If you mean the external element synchronization, that's been released in the latest version |
Beta Was this translation helpful? Give feedback.
-
oh thanks! I will try that! I was expecting this issue to be referenced or something similar I guess :) |
Beta Was this translation helpful? Give feedback.
-
@atomiks How would you deal with forwarded refs (i.e. Here is some code:
Or is it not possible to use a ref? Also how does one set prop getter on a ref? https://floating-ui.com/docs/react#prop-getters I can't find how to do it in the docs. Her eis a sandbox, useClick doesn't seem to work: https://codesandbox.io/s/floating-ui-useclick-not-working-s7ct31 |
Beta Was this translation helpful? Give feedback.
-
@basickarl you're describing a React data flow problem at its core (or even just a scoping problem really), and there's an open issue here: #2128 Let's imagine you manually wanted to put Generally there are five patterns that work in this scenario:
const [getReferenceProps, setGetReferenceProps] = useState();
return <>
<button {...getReferenceProps?.()} />
<Tooltip setGetReferenceProps={setGetReferenceProps} />
</>; useLayoutEffect(() => {
onGetReferencePropsChange(() => getReferenceProps);
}, [getReferenceProps]); In this case make sure you're using state too, not a plain ref, to externally synchronize the element (second code block here). There may be more, or a dedicated way to improve this scenario. Let me know if you have any ideas. |
Beta Was this translation helpful? Give feedback.
-
Converting this to a discussion since the original parts of this issue are already implemented now. |
Beta Was this translation helpful? Give feedback.
-
Hello, @atomiks! |
Beta Was this translation helpful? Give feedback.
-
Hey again! Adding one more thing. I'm implementing a listbox right now and trying to use
These hooks currently operate on indices, but listboxes (or selects, etc) as form components usually want to operate based on values. I guess the indices work nicely for menus where there is no "value". This is pretty obvious from the composable select example as well, as a "real" select component would also have a way of specifying defaultValue/value/onChange, which should be related to value props on options. So, to make it work with values, it is currently necessary to manually create a mapping between the indices and values, and then derive one or the other from the controlled value passed into the component. One small but useful improvement would be to allow collecting arbitrary data with FloatingList/useListItem in addition to the current elements & labels. So then the options data (value, label, icon, etc) would be collected to the list level automatically, deriving the index from there is quite easy. But supporting "value"-base selection as an alternative could be even better. |
Beta Was this translation helpful? Give feedback.
-
All of the enhancements that were part of this original post are now implemented. I'm going to convert this to a discussion if you want to talk about other API changes for v1.
Beta Was this translation helpful? Give feedback.
All reactions