-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix misplaced Popover when using Floating UI (#566) #576
Conversation
e592d2c
to
05094c7
Compare
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.
👍
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.
In the end, after Monday's discussion, I do not see this solution satisfying.
Floating UI is just example we use just because Floating UI is currently trending library that solves positioning. But we are open to every implementation user decides to use. This means that we are unable to determine attributes user would need to pass, position, x, y are attributes that are necessary for our example but might not be for production use.
I want @adamkudrna to decide this together with us. I would personally suggest to implement something like style
. I would call it e.g. placementStyle
.
placementStyle
could be either just the way how to bypass styles (style={placementStyle}
) or it can have black or white list or style attributes to pass.
I know that React 19 will get rid of forwardRef
, but I see this concept of forward-ref (respectively forward-styles) useful as we can control it for the component we know it use useful.
This is my proposal.
I can also imagine that we would require to pass either placement
or placementStyle
, so that only one can be set at the time. But this is just question arising in my head.
I agree with @bedrich-schindler that we do not want to embrace Floating UI to closely. I think the question is if some other placement implementation is likely to express position differently then x, y and position. I don't feel versed enough in CSS to decide this. I would think it less likely, but I'm don't really know.
So you mean to have one new prop: placement: PropTypes.shape({
/**
* This sets the CSS property `position` of the popover. This is reserved for use with Floating UI.
*/
position: PropTypes.string,
/**
* This sets the CSS property `top` of the popover. This is reserved for use with Floating UI.
*/
x: PropTypes.string,
/**
* This sets the CSS property `left` of the popover. This is reserved for use with Floating UI.
*/
y: PropTypes.string,
}) so we can then easily switch between placement formats? |
|
So something like:
where the signature of Floating UI implementation would be:
with the return value being the style HTML attribute value? |
On call we agreed that the best solution is from @bedrich-schindler
We would like to filter the style attributes to allow all properties that are neede for:
We need to reflect this in the docs. |
In case a general positioning white-list of relevant CSS properties is needed, this is what I can think of:
Notes (could be mentioned in the docs):
https://developer.mozilla.org/en-US/docs/Web/CSS/inset I am leaving out these:
|
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 tried to test it in the browser and the Floating UI example works, but the others above are all broken.
45c0bc3
to
8f8b5d6
Compare
01abd9f
to
c1a8cb3
Compare
c1a8cb3
to
e8d4e6e
Compare
e8d4e6e
to
862225c
Compare
Closes #566