Fix bug where a position: fixed popover content doesn't scroll #1064
Conversation
There was a problem hiding this comment.
I think this is really cool but I don't think having the popover move position is a desirable UX.
If I open a popover which contains a form and then I scroll then I think I either a) want to see more content on the page to inform my decision about interacting with the form or b) want the popover to go away because I'm moving my focus to something else.
To step into the mind of the user... in either case, I don't feel in control of the UI if the popover is moving around, trying to stay in view. I'm used to being in control of the UI, and taking the action of scrolling to change the content that's in view. So if I want the popover to be in view, I'll just scroll to it.
I think we need to decide whether users are more a or b, above. There always be exceptions to either one. I think b is the more pleasant experience because in most cases when I scroll, I'm progressing through the UI, not trying to cross-reference two different parts of it.
|
@cchaos Correct me if I'm wrong but in the cross-referenced issue, it seems like a fixed position element with a popover is more of a special case then a common one right? The only cases where I can think of this being needed is for the app header and for a sidebar like in Canvas. If so, then maybe it merits a prop for turning on the "stay visible while scrolling" behavior. |
|
I'm fine with making it an optional prop, but it is a necessary fix. |
|
@cjcenizal @cchaos added |
| @@ -204,5 +207,22 @@ export const PopoverExample = { | |||
| </div> | |||
| ), | |||
| demo: <PopoverHTMLElementAnchor />, | |||
| }, { | |||
| title: 'Popover on a static element', | |||
| text: ( | ||
| <div> | ||
| <p> | ||
| Popover content even works on <EuiCode>position: static;</EuiCode> elements. |
| ); | ||
|
|
||
| return ( | ||
| <EuiPopover |
There was a problem hiding this comment.
In the docs I'd provide a separate button that says "Show fixed popover example", that then toggles the fixed button display so it isn't always shown.
This falls in line with how we handle other fixed content examples in the docs like modals, fixed progress bars, the bottom bar...etc.
There was a problem hiding this comment.
This is probably the best example of doing something like that.
src/components/popover/popover.js
Outdated
| @@ -421,6 +435,7 @@ EuiPopover.propTypes = { | |||
| PropTypes.node, | |||
| PropTypes.instanceOf(HTMLElement) | |||
| ]), | |||
| repositionOnScroll: PropTypes.bool, | |||
There was a problem hiding this comment.
Add an autodoc for what this does.
|
@snide updated with your requested changes |
Fixes #1059 by updating the popover's content position when the page scrolls. The added example may be too obnoxious.
As a fun side-effect, this means all popover content repositions with scrolling.