Layer: now supports React context passing through, which affects all Layered components.#659
Conversation
…essary code, adding test coverage.
…render if targeted LayerHost is added/removed asynchrnously.
joschect
left a comment
joschect
left a comment
There was a problem hiding this comment.
Overall looks good, I added some nits and suggestions.
| /** | ||
| * Callback for when the layer is mounted. | ||
| */ | ||
| onLayerDidMount?: () => void; |
There was a problem hiding this comment.
Tentative suggestion: If you're going this route you might want to add a few other life cycle options, like onLayerWillMount().
| private _hasMounted: boolean; | ||
| /** | ||
| * Used for notifying applicable Layers that a host is available/unavailable and to re-evaluate Layers that | ||
| * case about the specific host. |
There was a problem hiding this comment.
Nit typo should be "care" not "case"
| /** | ||
| * The id property provided on a LayerHost that this Layer should render within. | ||
| */ | ||
| hostId?: string; |
There was a problem hiding this comment.
Why not have this be host?: string | htmlelement
There was a problem hiding this comment.
Rename to to hostselector and add support for .querySelector.
There was a problem hiding this comment.
Avoiding this one because we need the layerhost id and layer's hostid to match.
| let doc = getDocument(this._rootElement); | ||
|
|
||
| if (hostId) { | ||
| return doc.getElementById(hostId); |
There was a problem hiding this comment.
What if the element returned is null? It's unlikely and we expect them to pass in a good value but it could cause problems.
Also I think you need the conditional to be if (hostId && doc) to check for ssr.
There was a problem hiding this comment.
I'm explicitly handling this... did you see the example?
There was a problem hiding this comment.
look at the _getHost call. It can return null and it's handled.
…Layered components. (microsoft#659) * Fixing layer to respect context. * Simplifying layer, adjusting to support react context, removing unnecessary code, adding test coverage. * Adding change file. * Fixing lint * Fixing test. * Adjusting LayerHost to be simpler and Layer to support conditionally render if targeted LayerHost is added/removed asynchrnously. * fixing lint. * Fixing comments.
3 participants
Pull request checklist
npmx changeto generate it.)Description of changes
Layers were not passing through React context, which prevented a number of integration scenarios that depend on context (like using React router or redux dispatcher accessed from context.)
Focus areas to test
All layered components should continue to function and render as expected.