Fix portal/popover position and scrolling issues #326
Conversation
overlay wrapper. Also undo previous stuff.
| // thus, we pass a fake HTML bodyElement to Tether, | ||
| // with a no-op `appendChild` function (the only | ||
| // function the library uses from bodyElement.) | ||
| const fauxHtmlElement = ({ |
There was a problem hiding this comment.
Please avoid using "faux" in the terminology and instead opt for a more straightforward name, fakeBodyElement. It's harder to follow for non-native English speakers. -1 for this
| @@ -14,6 +14,19 @@ const DEFAULT_CONSTRAINTS = { | |||
| to: "scrollParent", | |||
| }; | |||
|
|
|||
| // per https://github.com/HubSpot/tether/pull/204, | |||
There was a problem hiding this comment.
minor: I think the line breaks are a little too aggressive here -- you could comfortably take up more horizontal space
There was a problem hiding this comment.
so true! at least 80 characters, could go up to 💯.
There was a problem hiding this comment.
longer lines will make these comments look less wordy too 👒
| // take the portal out of the document flow to prevent | ||
| // browsers from autoscrolling to the bottom of the document | ||
| // (where portals are appended) when programmatically | ||
| // .focus()'ing within a portal child element. also, don't |
There was a problem hiding this comment.
minor: don't need to say ".focus()", just use plain English, e.g. "focussing"
|
If the DefinitelyTyped PR doesn't merge in the next couple days, I think you should go ahead with this PR for this week's release (but I think they're pretty quick and I already reviewed it, so you should be 👌 ) |
|
@adidahiya the DefinitelyTyped PR hasn't merged yet. How do we move ahead with this PR properly in the meantime? |
|
@cmslewis you should be able to merge namespaces and add the field you need |
|
@adidahiya in which file? declaration merging can only be done in |
|
Chrome |
|
@giladgray yes, you can add a |
|
Verified on Chrome (v55) + Firefox + Safari on OS X and IE + Chrome on Windows. |
Fixes #231, Fixes #240
Changes proposed in this pull request:
.pt-portaltoposition:absoluteto take it out of document flow.This prevents browsers from autoscrolling to the bottom of the document (where portals are appended) when programmatically .focus()'ing within a portal child element.
.pt-portalto the top, left, and right of thebody.This ensures that content still calculates offsets from the top of the document and that content won't be horizontally scrunched.
tabindexfrom the.pt-overlaywrapper down to the overlay's immediate children.Despite the CSS changes above, programmatically
.focus()'ing on the overlay can still cause autoscrolling to the bottom of the page in some cases. Moving thetabindexto the overlay content ensures that the browser will scroll to the content itself if need be, not to some hidden parent element.This helps us solve an issue wherein Tether was removing portal content nodes and reinserting them as immediate children of the
bodyif any ancestor was notposition: static. This in turn severed React's handles on the original DOM nodes.Reviewers should focus on:
bodyElementoption.fakeHtmlElement.