Skip to content

feat(renderer): add targetDocument option#703

Closed
layershifter wants to merge 2 commits intorobinweser:masterfrom
layershifter:feat/target-document
Closed

feat(renderer): add targetDocument option#703
layershifter wants to merge 2 commits intorobinweser:masterfrom
layershifter:feat/target-document

Conversation

@layershifter
Copy link
Copy Markdown
Contributor

Fixes #693.

Description

This PR adds an targetDocument option to createRenderer() that allows to render styles properly to child windows: microsoft/fluent-ui-react#1178

Packages

  • fela
  • fela-dom

Versioning

Minor

Checklist

Quality Assurance

  • The code was formatted using Prettier (yarn run format)
  • The code has no linting errors (yarn run lint)
  • All tests are passing (yarn run test)
  • There are no flow-type errors (yarn run flow)

Changes

  • Tests have been added/updated
  • Documentation has been added/updated
  • My changes have proper flow-types

nodes: Object,
score: number,
{ type, media, support }: NodeAttributes,
targetDocument: Document,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order of params is changed because targetDocument is required

@layershifter
Copy link
Copy Markdown
Contributor Author

layershifter commented Apr 23, 2019

This PR actually adds changes to renderer 👎

Another approach is:

  • accept target prop on RendererProvider
  • pass it to rehydrate() and render() as second param
  • pass it to createSubscription() and getNodeCache()

But, in this case we will hit cache issues (render() and renderer.updateSubscription()), see #704. I tried it in microsoft/fluent-ui-react#1252 and works much better for us 🚀

@rofrischmann can I hear your feedback? 👂

@layershifter
Copy link
Copy Markdown
Contributor Author

Closing in favor of #704.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support style injection target in renderer

1 participant