Adding partial support for internal scrolling - #539
Conversation
…elstr/popup-scroll/372
There was a problem hiding this comment.
What are the scenarios we are 100% sure we want to address? If it's just the matter of the popup looking weird when it's not visibly attached to anything, then we should probably check containment of the anchor.
Let's say, we have a scrollable horizontal menu, and each item is a dropdown. It's totally fine for the popups to be outside of the scroll container. Things only start to look weird when the menu item itself starts to leave the container.
It's also interesting to look at how native components deal with this problem, because we might want to be able to replicate their behavior using our Popup.
Here's an example http://jsfiddle.net/vLdknthc/
Chrome and Firefox don't adjust the popup position no matter what happens. Chrome blocks user-initiated scrolling on the entire page while the popup is open, and Firefox dismisses the popup when you try to scroll.
|
|
||
| const rect = this.portal && this.portal.getPortal && this.portal.getPortal()!.getBoundingClientRect(); | ||
| if (rect) { | ||
| this.isOutOfBounds = isOutOfBounds(newStyle.top, newStyle.left, rect.height, rect.width) ? true : false; |
There was a problem hiding this comment.
My first thought was "out of bounds of what?". OK, out of bounds of the window. But fully or partially?
I suggest renaming to isFullyContainedWithinWindow() or something along those lines.
Also probably don't need ternary here.
There was a problem hiding this comment.
Yeah, not sure why I used one here.. refactor leftovers or lack of sleep?
| } | ||
|
|
||
| public getPortal(): Element | null { | ||
| if (this.container && this.container.children.length > 0) { |
| }; | ||
|
|
||
| private portal: Portal | null; | ||
| private isOutOfBounds = false; |
There was a problem hiding this comment.
Since this variable does not always reflect the current state and only becomes true during a transition, maybe rename to isExitingBounds?
| if (isOutOfBounds(this.props.anchor.y, this.props.anchor.x, 0, 0)) { | ||
| this.forceUpdate(); | ||
| } | ||
| } else if ((e.target as Node).contains(this.props.anchor)) { |
There was a problem hiding this comment.
Not that I think it's a problem, just worth mentioning: contains is inclusive, i.e. x.contains(x) is true, so we will also update when the anchor itself is being scrolled.
There was a problem hiding this comment.
It is a problem. We don't want to re-render the popup in this situation.
| } | ||
|
|
||
| private onScroll = (e: Event) => { | ||
| if (this.props.anchor) { |
There was a problem hiding this comment.
Should we also check that the popup is open?
|
|
||
| private onScroll = (e: Event) => { | ||
| if (this.props.anchor) { | ||
| if (isPoint(this.props.anchor)) { |
There was a problem hiding this comment.
There is a bug here. I need to figure out what to do if the anchor is a point.
| } | ||
|
|
||
| public getPortal(): Element | null { | ||
| if (this.container && this.container.children.length > 0) { |
There was a problem hiding this comment.
firstElementChild also here?
| if (this.props.anchor && this.props.open) { | ||
| if (e.target === document || isPoint(this.props.anchor)) { | ||
| const rect = this.getPortalRect(); | ||
| if (rect && isFullyContainedWithinWindow(rect.top, rect.left, rect.height, rect.width)) { |
There was a problem hiding this comment.
Shouldn't isFullyContainedWithinWindow be negated here?
It seems that two forceUpdate calls here serve different purposes: 1) triggering outOfBounds event (in which case I'm not sure why we don't call it directly without forcing update), and 2) updating the popup position. Maybe you could add a comment to explain why we treat these two cases differently, i.e. check out of bounds in one case but not in the other?
There was a problem hiding this comment.
It should, but this code isn't working anyway, I just didn't fix it yet..
JoMarton
left a comment
JoMarton
left a comment
There was a problem hiding this comment.
Add horizontal scrolling test
Also add a test for getPortal in Portal component
| }); | ||
|
|
||
| it('calls onExitBounds when the popup leaves the viewport', async () => { | ||
| let div: HTMLDivElement; |
There was a problem hiding this comment.
please rename this....very confusing with actual html div
| scrollDiv!.scrollTop = 51; | ||
|
|
||
| return waitFor(() => { | ||
| expect(onExitBounds).to.have.been.called; |
There was a problem hiding this comment.
should it be called once? on every scroll? what happens when it comes back into bounds?
| <div style={{height: '50px'}}>Body</div> | ||
| </Popup>).withDriver(PopupTestDriver); | ||
|
|
||
| await waitForDom(() => expect(popup.root).to.be.present()); |
There was a problem hiding this comment.
I would add tests to verify the expected position of the popup.
Also you could verify that the outOfBound event wasn't fired yet
| return newStyle; | ||
| } | ||
|
|
||
| private getPortalRect(): ClientRect | null { |
There was a problem hiding this comment.
to be more exact it is the rect of the portal contents
| } | ||
| } | ||
|
|
||
| public getPortal(): Element | null { |
There was a problem hiding this comment.
We need to further explore if this is the best solution
| return elem.hasOwnProperty('x') && elem.hasOwnProperty('y'); | ||
| } | ||
|
|
||
| function isFullyContainedWithinWindow(top: number, left: number, height: number, width: number): boolean { |
There was a problem hiding this comment.
please have tests for this function
…elstr/popup-scroll/372
3 participants
Added support for internal scrolling and onExitBounds event.
see #372