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: Introduce portal-based slot / fill alternative, render Popover as slot / fill #2966

Merged
merged 3 commits into from Oct 16, 2017

Conversation

Projects
None yet
4 participants
@aduth
Member

aduth commented Oct 10, 2017

This pull request seeks to refactor Popover to simplify rendering as a slot / fill pair, rather than require a custom context provider providing a DOM node render target.

Implementation notes:

Originally I sought to implement this using react-slot-fill as we use elsewhere, but quickly encountered similar issues as discovered in #2734 where lifecycle of slot content is not always predictable since component updates are reflected only after a subsequent state change.

Recognizing that React 16's portals feature provides the tools for a Slot / Fill implementation, I sought to explore what would be necessary for a first-party implementation. Happily it seemed to work quite well, needing only a context provider to register, unregister, and retrieve ref nodes of the rendered Slots to which a Fill can create a portal. The first-party portal implementation does not suffer from the same lifecycle issues as react-slot-fill. Further encouraging is that such an approach appears advocated by Dominic Gannaway, a React core team member.

We should decide whether this is worth using as a substitute to react-slot-fill. It may be worthwhile to split this pull request into two, where the first would convert existing usage to the new WordPress-specific slot-fill.

Testing instructions:

Verify that there are no regressions in Popover usage, particularly:

  • Different types of popovers (tooltips, dropdown)
  • Focus should transition into popover when opened (e.g. Inserter)
  • Popover should reorient itself when lacking sufficient space to open in the desired direction
  • Popover should close when clicking outside
  • No console errors should be logged in any of the above

@aduth aduth added the UI Components label Oct 10, 2017

@codecov

This comment has been minimized.

codecov bot commented Oct 10, 2017

Codecov Report

Merging #2966 into master will decrease coverage by 0.24%.
The diff coverage is 15.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2966      +/-   ##
=========================================
- Coverage   33.15%   32.9%   -0.25%     
=========================================
  Files         200     202       +2     
  Lines        5836    5883      +47     
  Branches     1031    1040       +9     
=========================================
+ Hits         1935    1936       +1     
- Misses       3293    3330      +37     
- Partials      608     617       +9
Impacted Files Coverage Δ
editor/index.js 0% <ø> (ø) ⬆️
blocks/inspector-controls/index.js 88.88% <ø> (ø) ⬆️
blocks/block-controls/index.js 0% <ø> (ø) ⬆️
blocks/editable/index.js 10.5% <ø> (ø) ⬆️
editor/block-toolbar/index.js 0% <0%> (ø) ⬆️
editor/sidebar/block-inspector/index.js 0% <0%> (ø) ⬆️
editor/provider/index.js 0% <0%> (ø) ⬆️
editor/layout/index.js 0% <0%> (ø) ⬆️
components/slot-fill/provider.js 5% <5%> (ø)
components/slot-fill/fill.js 7.14% <7.14%> (ø)
... and 7 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 7c3bcdd...1fbb24f. Read the comment docs.

@youknowriad

Such a great code :). Simple and efficient.
Agreed we should replace our current slot and fill usage.

Before or after this PR, doesn't matter that much for me.

@@ -8,6 +8,8 @@ import { mount } from 'enzyme';
*/
import Dropdown from '../';

jest.mock( '../../slot-fill/fill' );

This comment has been minimized.

@gziolo

gziolo Oct 11, 2017

Member

I'm surprised it works :)
My experience so far was that Jest expected absolute paths everywhere. The nice thing is that it warns when it isn't able to locate a file that it should mock.

@aduth

This comment has been minimized.

Member

aduth commented Oct 13, 2017

Rebased to resolve conflicts. I started toying locally to see whether the custom implementation would work well for our other use of Slot / Fill and for the most part it does. One issue was if a fill is rendered before the slot is mounted. The updated implementation now tracks all fills and forces a re-render should a slot become registered for that name. Further, I made the provider context optional for slot and fills, avoiding the need to mock tests on Fill (default behavior will render children verbatim in-place).

Finally, since I did the work... I decided to just finish dropping react-slot-fill here 😐

@aduth

This comment has been minimized.

Member

aduth commented Oct 14, 2017

Further, I made the provider context optional for slot and fills, avoiding the need to mock tests on Fill (default behavior will render children verbatim in-place).

Hmm, this appears to be problematic, since for something like block inspector, if the sidebar panel isn't opened, the inspector controls will render adjacent the preview, rather than not render at all. Probably need to revert this specific change.

@aduth

This comment has been minimized.

Member

aduth commented Oct 16, 2017

This grew a bit larger than I would have liked, but it should be pretty well settled now. In recent changes:

  • Reverted to not rendering a fill if context is not available, but kept revisions to avoid errors being thrown in the process
  • Updated Popover to check whether slot context exists. This is a workaround to preserve previous portal behavior (which, frankly, probably wouldn't have worked after a React 16 upgrade), and from a developer experience improves usability of the Popover component to be slot-optional (first iterations of the popover rendered in-place).
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 16, 2017

Still working great 👍

aduth added some commits Oct 10, 2017

Components: Create custom slot-fill component using React createPortal
Components: Track fill registration

Since slot isn't guaranteed to be mounted at time of initial fill render, track fills so that when slot does become available, we can re-render each fill

Components: Remove provider requirement for Slot Fill

Render children verbatim if no context available

Components: Avoid rendering fill if no slot context
Components: Render popover as slot / fill
Components: Check popover context before rendering to Fill

@aduth aduth force-pushed the update/popover-slot branch from bdbb2a6 to 1fbb24f Oct 16, 2017

@aduth aduth merged commit 8d8e790 into master Oct 16, 2017

3 checks passed

codecov/project 32.9% (-0.25%) compared to 7c3bcdd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the update/popover-slot branch Oct 16, 2017

@mtias

This comment has been minimized.

Contributor

mtias commented Oct 18, 2017

Nice work here, @aduth.

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