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

Fix misplaced Popover when using Floating UI (#566) #576

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

atmelmicro
Copy link
Collaborator

@atmelmicro atmelmicro commented Nov 19, 2024

Closes #566

@atmelmicro atmelmicro force-pushed the bugfix/misplaced-popover-floating-ui branch from e592d2c to 05094c7 Compare November 19, 2024 09:15
@atmelmicro atmelmicro changed the title Fix misplaced when using Floating UI (#566) Fix misplaced Popover when using Floating UI (#566) Nov 19, 2024
Copy link
Contributor

@mbohal mbohal left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@bedrich-schindler bedrich-schindler left a 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.

@mbohal
Copy link
Contributor

mbohal commented Nov 21, 2024

But we are open to every implementation user decides to use.

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.

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.

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?

@adamkudrna
Copy link
Member

  • I can imagine positioning solved via CSS transforms (translateX, translateY properties). I think the new drag-and-drop library we use does it this way as it is more performant and fluent.
  • Can we have something like adapters for supported positioning libraries? We would provide the Floating UI adapter and anyone could plug-in their own.

@mbohal
Copy link
Contributor

mbohal commented Nov 30, 2024

Can we have something like adapters for supported positioning libraries? We would provide the Floating UI adapter and anyone could plug-in their own.

So something like:

getPlacement: PropTypes.func,

where the signature of Floating UI implementation would be:

getPlacement(x: number, y: number, position: string): { x: number, y: number, position: string }

with the return value being the style HTML attribute value?
I like that.

@mbohal
Copy link
Contributor

mbohal commented Dec 2, 2024

On call we agreed that the best solution is from @bedrich-schindler

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.

We would like to filter the style attributes to allow all properties that are neede for:

  • absolute positioning
  • CSS transform

We need to reflect this in the docs.

@adamkudrna
Copy link
Member

adamkudrna commented Dec 2, 2024

In case a general positioning white-list of relevant CSS properties is needed, this is what I can think of:

position
inset
inset-inline-start
inset-inline-end
inset-block-start
inset-block-end
top
right
bottom
left
translate
transform-origin

Notes (could be mentioned in the docs):

  • ⚠️ inset is a shorthand for top right bottom left, not for inset-* properties.
  • As opposed to top right bottom left and the inset shorthand, inset-* properties are writing-direction aware.

https://developer.mozilla.org/en-US/docs/Web/CSS/inset

I am leaving out these:

  • inset-inline, inset-block: if a shorthand is needed, I would go with inset (although it's not writing-direction aware).
  • transform: it allows to set any transforms including rotation and distortion, not only x/y(/z) translation. That seems to be too powerful to me.

Copy link
Member

@adamkudrna adamkudrna left a 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.

@atmelmicro atmelmicro force-pushed the bugfix/misplaced-popover-floating-ui branch from 45c0bc3 to 8f8b5d6 Compare January 23, 2025 16:26
@atmelmicro atmelmicro force-pushed the bugfix/misplaced-popover-floating-ui branch 2 times, most recently from 01abd9f to c1a8cb3 Compare January 31, 2025 14:55
@atmelmicro atmelmicro force-pushed the bugfix/misplaced-popover-floating-ui branch from c1a8cb3 to e8d4e6e Compare January 31, 2025 14:56
@atmelmicro atmelmicro force-pushed the bugfix/misplaced-popover-floating-ui branch from e8d4e6e to 862225c Compare January 31, 2025 15:00
@atmelmicro atmelmicro merged commit bf92ba4 into master Jan 31, 2025
10 of 11 checks passed
@atmelmicro atmelmicro deleted the bugfix/misplaced-popover-floating-ui branch January 31, 2025 16:39
@adamkudrna adamkudrna added the fix Fixing a bug label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misplaced Popover when used with Floating UI
4 participants