[WIP] - Portal component experiment #37
Conversation
miroslavstastny
changed the title
|
Why we need An update for |
src/components/Portal/Portal.tsx
Outdated
| handleTriggerClick = () => { | ||
| debug('handleTriggerClick()') | ||
| this.props.trigger.props.onClick() | ||
| this.setState({ open: !this.state.open }) |
There was a problem hiding this comment.
Since open is an auto-controlled, it should be set with this.trySetState(). This method is added by AutoControlledComponent and ensures props and default props are respected.
There was a problem hiding this comment.
Done. Also marked the second parameter on AutoControlledComponent.trySetState() as optional.
|
|
||
| handleTriggerClick = () => { | ||
| debug('handleTriggerClick()') | ||
| this.props.trigger.props.onClick() |
There was a problem hiding this comment.
This will throw if the trigger does not have an onClick prop. Lodash provides a method for safely invoking a path to a function that may or may not exist. We also need to pass the original event back on click:
handleTriggerClick = (e) => {
_.invoke(this.props.trigger, 'onClick', e)
}|
|
||
| handlePortalMouseEnter = () => { | ||
| debug('handlePortalMouseEnter()') | ||
| } |
src/components/Portal/Portal.tsx
Outdated
| if (this.state.portalEl) { | ||
| return | ||
| } | ||
| console.log('creating portalEl') |
There was a problem hiding this comment.
No log calls. Use debug() instead. You can enable it from the console like so:
localStorage.debug = 'semanticUIReact:portal'
// refresh browserNote, we should update the debugger namespace in
debug.tstostardust.
| })} | ||
| {this.state.open && | ||
| this.state.portalEl && | ||
| ReactDOM.createPortal(this.props.children, this.state.portalEl)} |
There was a problem hiding this comment.
Let's stick with destructuring props and state at the top of our functions. It breaks object references and makes code more readable:
render() {
const { open, portalEl } = this.state
// ...
}| // To discuss: | ||
| // when to create rootNode? (it is required in render, componentWillMount is deprecated) | ||
| // should multiple portals share it? (how would mouseenter/mouseleave on portalEl work then?) | ||
| // when to destroy it (it is too early in componentWillUnmount) |
There was a problem hiding this comment.
I'm not certain on how we should handle the root node, however, we can use the constructor for a componentWillMount replacement. Could you explain a bit about why componentWillUnmount is too early for removing the mount node?
There was a problem hiding this comment.
In componentWillUnmount all the elements which render() created are still in DOM, that means that the mount node still contains the popup.
I was afraid I would confuse React by removing it's elements from DOM directly. But React seems to be fine with that.
| <Button | ||
| onClick={() => { | ||
| console.log('onClick outer') | ||
| }} |
There was a problem hiding this comment.
Avoid creating functions in the render method as it is not a best practice. Treat functions as persisted state and hoist them up to methods. This way, they are the same instance between renders and do not void any equality checks between renders.
The new Portal in SUIR does offer an interesting approach. Check the render method. What do you think @miroslavstastny? |
Replacing |
|
Two main takeaways from the Semantic-Org#2880:
@layershifter, @levithomason - what's your opinion on using |
|
As default value, it's okay I think. Possible, @levithomason has another opinion. |
|
Discussed with @levithomason, we will reuse the new implementation from SIUR. |
levithomason
force-pushed
the
master
branch
5 times, most recently
from
f4eb5e5
to
339716d
Compare
|
Ping, let's move this to the new repo. |
Portal component implementation using React Portal