-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Portal] SSR support, one fewer element in Overlay #2205
Conversation
- created DOM element receives className - remove containerRef prop and associated element
- use TransitionGroup as the wrapper element!!
@@ -144,7 +145,7 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> { | |||
// an HTMLElement that contains the backdrop and any children, to query for focus target | |||
public containerElement: HTMLElement; | |||
private refHandlers = { | |||
container: (ref: HTMLDivElement) => (this.containerElement = ref), | |||
container: (ref: React.ReactInstance) => (this.containerElement = findDOMNode(ref) as HTMLElement), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this now receives the Portal
instance, so we unwrap it into underlying DOM node.
refactor TransitionGroup usagePreview: documentation | landing | table |
// CSS position mode under the hood. also, make the container focusable so we can | ||
// trap focus inside it (via `enforceFocus`). | ||
const decoratedChild = | ||
typeof child !== "object" ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefer reversing these conditions. read as if (truthy looking thing) ... else
@@ -31,7 +24,7 @@ export interface IPortalState { | |||
} | |||
|
|||
export interface IPortalContext { | |||
/** Additional class to add to portal element */ | |||
/** Additional CSS classes to add to all `Portal elements in this React context. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing trailing backtick in docs
// Only render `children` once this component has mounted in a browser environment, so they are | ||
// immediately attached to the DOM tree and can do DOM things like measuring or `autoFocus`. | ||
// See long comment on componentDidMount in https://reactjs.org/docs/portals.html#event-bubbling-through-portals | ||
return typeof document !== "undefined" && this.state.hasMounted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a little too clever to read as a ternary, please make it more verbose w/ if () else
. I think spelling out the 3 cases is quite useful:
if (typeof document === "undefined") {
return null;
} else if (!this.state.hasMounted) {
return null;
} else {
...
}
} | ||
} | ||
|
||
private createElement() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: createContainerElement
describe("Core isomorphic rendering", () => { | ||
generateIsomorphicTests(Core, customProps, customChildren, skipList); | ||
generateIsomorphicTests(Core, customProps, customChildren); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 💯
ternary style refactorsPreview: documentation | landing | table |
Merge branch 'develop' of github.com:palantir/blueprint into gg/portal-ssrPreview: documentation | landing | table |
Fixes #2191
Changes proposed in this pull request:
(ok sorry there are some breaking changes here, but I promise they're worth it)
Portal
to support SSR by checking for browser environment and creating element inDidMount()
instead of constructorPortal
is now treated as its root, soclassName
is applied to this element. this also means that no dummy element is created inside the portal, making it one level shallower.containerRef
prop has been removed as it no longer makes sense. users can simply apply refs to the elements they render in the portal.Overlay
to not usecontainerRef
TransitionGroup
as the wrapper element removes one empty element from the DOM tree:before:
after: