Skip to content

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

Merged
atmelmicro merged 1 commit into
masterfrom
bugfix/misplaced-popover-floating-ui
Jan 31, 2025
Merged

Fix misplaced Popover when using Floating UI (#566)#576
atmelmicro merged 1 commit into
masterfrom
bugfix/misplaced-popover-floating-ui

Conversation

@atmelmicro

@atmelmicro atmelmicro commented Nov 19, 2024

Copy link
Copy Markdown
Contributor

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

@mbohal mbohal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@bedrich-schindler bedrich-schindler left a comment

Copy link
Copy Markdown
Contributor

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

mbohal commented Nov 21, 2024

Copy link
Copy Markdown
Contributor
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
Copy Markdown
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

mbohal commented Nov 30, 2024

Copy link
Copy Markdown
Contributor

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

mbohal commented Dec 2, 2024

Copy link
Copy Markdown
Contributor

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

adamkudrna commented Dec 2, 2024

Copy link
Copy Markdown
Member

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.

Comment thread src/components/Popover/README.md Outdated
Comment thread src/components/Popover/_helpers/cleanPlacementStyle.js Outdated

@adamkudrna adamkudrna left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/components/Popover/README.md Outdated
@atmelmicro
atmelmicro force-pushed the bugfix/misplaced-popover-floating-ui branch from 45c0bc3 to 8f8b5d6 Compare January 23, 2025 16:26
Comment thread src/components/Popover/Popover.jsx Outdated
Comment thread src/components/Popover/README.md Outdated
@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
@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