Skip to content
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

Components: Allow style cascade to Popovers by rendering into Gutenberg root element #2301

Merged
merged 3 commits into from
Aug 11, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Aug 9, 2017

Related: #2160
Related: #2291 (comment)

With #2160, popovers now render as a direct child of the <body> element, which has the advantage of escaping from style cascade of ancestor nodes, and the downside of escaping style cascade of ancestor nodes. Since some styles are considered "global" in the context of the Gutenberg editor (font sizes, colors, box-sizing, etc), these should still take effect on popover elements. The changes here effectively change the render target of popovers from document.body to the Gutenberg root element (#editor).

Implementation notes:

This adds yet another context provider, which in my cursory investigation seems to be the common approach for pre-React-16 portal implementations.

To try to remedy this issue, and with consideration of my own prior feedback at #2119 (comment), the approach here refactors editor rendered providers into a composable array of components.

Testing instructions:

Verify that there are no regressions in the behavior of Popover controls, currently present in:

  • Header inserter
  • Post settings visibility
  • Content inserter

Note that Gutenberg-global styles take effect in rendered popovers.

@aduth aduth added General Interface Parts of the UI which don't fall neatly under other labels. [Feature] UI Components Impacts or related to the UI component system labels Aug 9, 2017
@codecov
Copy link

codecov bot commented Aug 11, 2017

Codecov Report

Merging #2301 into master will increase coverage by 0.05%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2301      +/-   ##
==========================================
+ Coverage   25.69%   25.74%   +0.05%     
==========================================
  Files         154      155       +1     
  Lines        4811     4821      +10     
  Branches      810      812       +2     
==========================================
+ Hits         1236     1241       +5     
- Misses       3020     3024       +4     
- Partials      555      556       +1
Impacted Files Coverage Δ
editor/index.js 0% <0%> (ø) ⬆️
components/popover/index.js 94.91% <100%> (+0.17%) ⬆️
components/popover/provider.js 100% <100%> (ø)

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 960c0ae...4273abf. Read the comment docs.

@aduth aduth merged commit 44f97ab into master Aug 11, 2017
@aduth aduth deleted the fix/popover-cascade branch August 11, 2017 13:32
@aduth aduth mentioned this pull request Aug 11, 2017
@afercia
Copy link
Contributor

afercia commented Aug 12, 2017

change the render target of popovers from document.body to the Gutenberg root element (#editor).

Is there a way to specify a different target for a different type of popover? For example, if in the future the Popover component will be used for modal dialogs, they should be appended to the body element.

@aduth
Copy link
Member Author

aduth commented Aug 14, 2017

Is there a way to specify a different target for a different type of popover? For example, if in the future the Popover component will be used for modal dialogs, they should be appended to the body element.

It shouldn't be much work to implement. The changes here introduced React context specifying the target for popover rendering, so it applies to any popover in the editor. In specialized cases like modals, we could add a prop which would take precedence over this context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants