feat(portal): simple base implementation #144

bmdalex · 2018-08-27T15:29:20Z

Portal

This PR will introduce Portal component, a component that allows you to render children outside their parent (for now all portals are rendered at the end of the body DOM element); a Portal comes with the following props:

onMount?: (props: IPortalProps) => void: Called when the portal is mounted on the DOM
onUnmount?: (props: IPortalProps) => void: Called when the portal is unmounted from the DOM
open?: boolean: Controls whether or not the portal is displayed
trigger?: JSX.Element: Element to be rendered in-place where the portal is defined
triggerRef?: (node: HTMLElement) => void: Called with a ref to the trigger node

and the following helper components used by Portal:

PortalInner with props:
- onMount?: (props: IPortalProps) => void: Called when the portal is mounted on the DOM
- onUnmount?: (props: IPortalProps) => void: Called when the portal is unmounted from the DOM
Ref with props:
- innerRef?: (ref: HTMLElement) => void: Called with componentDidMount

TODO

Proposal

Basic portal

Renders a portal that is:

opened when clicking on the trigger, a component sent to the portal as a prop;
closed when clicking away or on the trigger (since it acts like a toggle)

<Portal
  content={<div>This is a basic portal</div>}
  trigger={<Button content={'Toggle portal'} onClick={this.handleClick} />}
/>

generates

<button>Toggle portal</button>

and at the end of body DOM element:

<body>
  ...
  <div>This is a basic portal</div>
</body>

Controlled portal

Renders a portal that is rendered or not depending on the value of open property

<Portal
  content={<div>This is a controlled portal</div>}
  open={true}
/>

generates

<body>
  ...
  <div>This is a controlled portal</div>
</body>

Toggle portal
at the end of body DOM element.

codecov · 2018-08-27T17:48:08Z

Codecov Report

Merging #144 into master will increase coverage by 0.94%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   89.23%   90.17%   +0.94%     
==========================================
  Files          50       55       +5     
  Lines         836      916      +80     
  Branches      130      142      +12     
==========================================
+ Hits          746      826      +80     
  Misses         86       86              
  Partials        4        4

Impacted Files	Coverage Δ
src/index.ts	`100% <100%> (ø)`	⬆️
src/components/Portal/PortalInner.tsx	`100% <100%> (ø)`
src/components/Ref/index.ts	`100% <100%> (ø)`
src/components/Ref/Ref.tsx	`100% <100%> (ø)`
src/components/Portal/index.ts	`100% <100%> (ø)`
src/components/Portal/Portal.tsx	`100% <100%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6d7bcb...d0df7ed. Read the comment docs.

levithomason · 2018-08-28T04:00:16Z

docs/src/examples/components/Portal/Types/PortalExample.shorthand.tsx

+    logCount: 0,
+  }
+
+  public render() {


I don't think we should be typing public/private like this in React components. Even though method and other custom methods are publicly accessible, it is not idiomatic React to actually access them. You should never reach into a React component and call a method.

In other words, if we did type them, it would be more proper to consider all methods as private but I believe that would cause some other issues.

I don't agree with this and here are my reasons:

The current pattern I see all over the project is to not specify any modifier for class members which means all members are public (class members are public by default in typescript).
This is wrong and is one of the main reasons I'd recommend enforcing modifiers; developers often forget to specify a method is private since no modifier is required and this makes it private.

render is a public method as it comes from React.Component or UIComponent, the classes we usually extend.

Specifying modifiers makes it clear for developers coming from other environments where private is the default class member modifier (such as C#).

We had this discussion before and since it's occurring again I can create an RFC for enforcing modifiers if you think it's valuable.

@levithomason @kuzhelov @miroslavstastny @mnajdova what do you guys think?

since this is an example I'll remove the modifiers so that we make it jsx, but the points stand for actual typescript code

levithomason · 2018-08-28T04:01:52Z

docs/src/examples/components/Portal/Types/PortalExampleControlled.tsx

@@ -0,0 +1,65 @@
+import * as React from 'react'


No typescript in examples please.

I'd rather allow both, the similar way as it is done on the Android Developer docs

then these should be jsx files, not tsx, right? :) I can do the change for now but it's confusing to see these tslint errors..

levithomason · 2018-08-28T04:04:51Z

src/components/Portal/PortalInner.tsx

@@ -0,0 +1,48 @@
+import * as PropTypes from 'prop-types'
+import { invoke } from 'lodash'


Let's keep convention consistent in the repo, it makes tooling and programmatic updates easier to manage.

There is a babel-plugin-lodash which we use at SUIR to rewrite the import statements to cherry pick only used methods. We should either implement this plugin, or enforce cherry picking lodash methods, but, we should not mix both patterns without a guarantee of either one.

levithomason · 2018-08-28T04:07:26Z

src/components/Portal/PortalInner.tsx

+  }
+
+  public render() {
+    return createPortal(this.props.children, document.body)


Accessing document will fail in server side rendering. This should probably be a defaultProp instead that is safe guarded by a browser check first:

import { isBrowser } from '../../lib' static defaultProps = { context: isBrowser() ? document.body : null } render() { const { children, context } = this.props return createPortal(children, context) }

I made the change but I don't really understand it. Server side rendering? Where do we do that?

levithomason · 2018-08-28T04:08:45Z

src/components/Portal/PortalInner.tsx

+  }
+
+  public componentDidMount() {
+    invoke(this.props, 'onMount', { ...this.props })


Since we're not adding / changing any props here, not sure that a shallow clone is required. We can save on the cycles by just passing this.props.

made the change as suggested

levithomason · 2018-08-28T04:11:07Z

docs/src/examples/components/Portal/Types/PortalExampleControlled.shorthand.tsx

+    log: [],
+    logCount: 0,
+    open: false,
+  }


In effort to minimize lines, this object can be single line:

state = { log: [], logCount: 0, open: false }

levithomason · 2018-08-28T04:11:38Z

docs/src/examples/components/Portal/Types/PortalExampleControlled.shorthand.tsx

+    const { log, logCount, open } = this.state
+    const content = open ? 'Close Portal' : 'Open Portal'
+
+    const controls = (


controls and portalContent can also be inlined into the render method to save some lines.

I think my approach makes it more clear how to actually use the components since the controls and portalContent are lengthy and users can get confused; that's why I did it like this... Anyway, I made the change as suggested

levithomason

Thanks for porting this. See individual comments for change request details.

bmdalex requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners August 27, 2018 15:29

bmdalex mentioned this pull request Aug 27, 2018

feat(portal): base implementation #115

Closed

11 tasks

bmdalex added the 🚧 WIP label Aug 27, 2018

bmdalex changed the title ~~[WIP] feat(portal): simple base implementation~~ feat(portal): simple base implementation Aug 27, 2018

bmdalex force-pushed the feat/portal-simple branch 2 times, most recently from b4d9467 to afa6847 Compare August 27, 2018 17:48

bmdalex added 🚀 ready for review and removed 🚧 WIP labels Aug 27, 2018

levithomason reviewed Aug 28, 2018

View reviewed changes

levithomason suggested changes Aug 28, 2018

View reviewed changes

bmdalex force-pushed the feat/portal-simple branch 5 times, most recently from a961ce4 to 83abf5b Compare September 4, 2018 09:01

sophieH29 mentioned this pull request Sep 4, 2018

[RFC] Extendable Portal functionality #180

Closed

bmdalex force-pushed the feat/portal-simple branch 3 times, most recently from 53987e1 to 4f1c8a1 Compare September 6, 2018 11:51

bmdalex force-pushed the feat/portal-simple branch from 4f1c8a1 to c8da43d Compare September 6, 2018 16:02

bmdalex added 2 commits September 7, 2018 15:56

feat(portal): simple base implementation

50bda74

addressed PR comments

d0df7ed

bmdalex force-pushed the feat/portal-simple branch from c8da43d to d0df7ed Compare September 7, 2018 13:56

levithomason added ready for merge and removed 🚀 ready for review labels Sep 7, 2018

levithomason approved these changes Sep 7, 2018

View reviewed changes

bmdalex merged commit a71db03 into master Sep 7, 2018

bmdalex deleted the feat/portal-simple branch September 7, 2018 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(portal): simple base implementation #144

feat(portal): simple base implementation #144

bmdalex commented Aug 27, 2018 •

edited

codecov bot commented Aug 27, 2018 •

edited

levithomason Aug 28, 2018 •

edited

bmdalex Aug 28, 2018

bmdalex Aug 28, 2018

levithomason Aug 28, 2018

smykhailov Aug 28, 2018

bmdalex Aug 28, 2018

levithomason Aug 28, 2018

bmdalex Aug 28, 2018

levithomason Aug 28, 2018

bmdalex Aug 28, 2018

levithomason Aug 28, 2018

bmdalex Aug 28, 2018

levithomason Aug 28, 2018

bmdalex Aug 28, 2018

levithomason Aug 28, 2018

bmdalex Aug 28, 2018

levithomason left a comment

		@@ -0,0 +1,48 @@
		import * as PropTypes from 'prop-types'
		import { invoke } from 'lodash'

feat(portal): simple base implementation #144

feat(portal): simple base implementation #144

Conversation

bmdalex commented Aug 27, 2018 • edited

Portal

TODO

Proposal

Basic portal

Controlled portal

codecov bot commented Aug 27, 2018 • edited

Codecov Report

levithomason Aug 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason left a comment

Choose a reason for hiding this comment

bmdalex commented Aug 27, 2018 •

edited

codecov bot commented Aug 27, 2018 •

edited

levithomason Aug 28, 2018 •

edited