[Popover] Scroll Container issue #7472
Conversation
gorkemcnr
commented
Jul 19, 2017
•
gorkemcnr
commented
- As discussed on [AutoComplete] DropDown Positioning #4393 and [Autocomplete] Port component #4783 I have provided a fix for popover position issue on autocomplete.
oliviertassinari
changed the title
oliviertassinari
added
the
PR: needs revision
src/Popover/Popover.js
Outdated
| @@ -395,7 +402,7 @@ class Popover extends Component { | |||
| return ( | |||
| <div style={styles.root}> | |||
| <EventListener | |||
| target="window" | |||
| target={this.props.scrollElement || "window"} | |||
There was a problem hiding this comment.
What about using a defaultProps instead?
src/Popover/Popover.js
Outdated
| @@ -89,6 +89,13 @@ class Popover extends Component { | |||
| * The zDepth of the popover. | |||
| */ | |||
| zDepth: propTypes.zDepth, | |||
| /** | |||
| * TODO | |||
There was a problem hiding this comment.
Can we have a description for the documentation?
-Prop name is changed to a more meaningful name
…com/gorkemcnr/material-ui into 4783-AutoCompletePopoverScrollIssue # Conflicts: # src/Popover/Popover.js
|
When you guys planning to release those changes? |
|
@gorkemcnr First, we need to get the build green to merge the PR. |
…com/gorkemcnr/material-ui into 4783-AutoCompletePopoverScrollIssue # Conflicts: # src/Popover/Popover.js
oliviertassinari
added
PR: Review Accepted
and removed
PR: needs revision
|
Thanks, we will try to do a release by this weekend. |
* call_em_all_-_master/master: fix: comment typo (mui#7523) [Docs] Add v0.18.7 to versions.json v0.18.7 [CHANGELOG] Prepare v0.18.7 [ListItem] Fix triggers onTouchTap when disabled (mui#7486) [Popover] Scroll Container issue (mui#7472) [Table] Don't set height to tbody (mui#7484) docs: fix typo in Avatar readme (mui#7478) # Conflicts: # src/Popover/Popover.js # merged both versions
| targetEl.style.top = `${Math.max(0, targetPosition.top)}px`; | ||
| targetEl.style.left = `${Math.max(0, targetPosition.left)}px`; | ||
| targetEl.style.top = `${targetPosition.top}px`; | ||
| targetEl.style.left = `${targetPosition.left}px`; |
There was a problem hiding this comment.
This change appears to be breaking large lists where the scrolling container (e.g. window) doesn't actually scroll. Previously prior to this change, having the 'Math.max' bit allowed the popover content to scroll rather than the scrolling container, but by allowing the popover to now extend beyond the bounds of the target element, this is broken. Could we perhaps have a new prop to bring back clamping to 0?
There was a problem hiding this comment.
I have created issue #7573 for this.
There was a problem hiding this comment.
@jamespizzurro Oh, we can always revert. I think that we will soon freeze the master branch. Fixes have been introducing too many regressions lately, the master doesn't have enough tests.
…om-build * fix/broken-touch-scroll-on-nested-menu-items: [Menu] Ignore loosing focus on click away for item with menu items [examples] Remove browserify Fixes mui#7559 (mui#7560) [docs] Update ROADMAP (mui#7543) [Table] Row click on empty cell to not die in IE (mui#7520) Internal(EnhancedSwitch): fix checked prop (mui#7499) fix: comment typo (mui#7523) [Docs] Add v0.18.7 to versions.json v0.18.7 [CHANGELOG] Prepare v0.18.7 [ListItem] Fix triggers onTouchTap when disabled (mui#7486) [Popover] Scroll Container issue (mui#7472) [Table] Don't set height to tbody (mui#7484) docs: fix typo in Avatar readme (mui#7478)
