At 4880 popover has incorrect orientation

mzbeetnoff commented 2 years ago

Fix issue with Popover component.

What The popover control previously was not detecting collisions properly. Correct the collision detection, and ensure we call closest scrollable on the target element to avoid clipping popover with edge of container.

Rename parent to container as it makes more sense with this naming.

Add new menu story to demo passing the parentRef to the popover control. Elements in Avail have not utilized this property yet, this storybook now demonstrates the correct usage by setting the ref on the target element. As we start to make Avail more accessible this will be very useful to ensure proper keyboard navigation.

Add unit tests

https://user-images.githubusercontent.com/106633502/189464994-9ea947f1-8918-4ea5-af0c-35ff6bccaf4f.mov

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

pkrawc commented 2 years ago

Will pull this down and review today!

mzbeetnoff commented 2 years ago

I'm not sure if this existed before or not but if there is no position relative on the body--assuming that's the container--then the position on the popover is calculated incorrectly.

closestScrollable should return the body if nothing lower is scrollable, but something weird (read I don't understand what) is happening with the box-model when no relative positioning is defined.

@pkrawc I believe I have corrected the issue. As far as I can tell the issue has existed for sometime, I was able to verify this by checking out an older Popover version here and using the example story you included to demonstrate it. It seems the issue is related to setting the margin value to something other than 0, and how the resize handler calculates the targetBounds.

mzbeetnoff commented 2 years ago

I'm not sure if this existed before or not but if there is no position relative on the body--assuming that's the container--then the position on the popover is calculated incorrectly.

closestScrollable should return the body if nothing lower is scrollable, but something weird (read I don't understand what) is happening with the box-model when no relative positioning is defined.

@pkrawc I believe I have corrected the issue. As far as I can tell the issue has existed for sometime, I was able to verify this by checking out an older Popover version here and using the example story you included to demonstrate it. It seems the issue is related to setting the margin value to something other than 0, and how the resize handler calculates the targetBounds.

mzbeetnoff commented 2 years ago

I'm not sure if this existed before or not but if there is no position relative on the body--assuming that's the container--then the position on the popover is calculated incorrectly. closestScrollable should return the body if nothing lower is scrollable, but something weird (read I don't understand what) is happening with the box-model when no relative positioning is defined.

@pkrawc I believe I have corrected the issue. As far as I can tell the issue has existed for sometime, I was able to verify this by checking out an older Popover version here and using the example story you included to demonstrate it. It seems the issue is related to setting the margin value to something other than 0, and how the resize handler calculates the targetBounds.

@pkrawc I reverted the change that fixed your popover example story because it was breaking the menu portal usage story and also because I feel the fix should be made in useResize. I'm kind of thinking we should revisit this issue separately in a later pull request / jira because it seems out of scope with this change. what do you think?

rentalutions / elements

At 4880 popover has incorrect orientation #201