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

update EuiOverlayMask to use react portal #254

Merged
merged 3 commits into from
Jan 3, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jan 2, 2018

Use react portal to mount the euiOverlayMask div directly to the body element to avoid situations where EuiOverlayMask does not cover entire screen. Below the kibana nav is on top of the euiOverlayMask div despite proper z-index settings.

screen shot 2018-01-02 at 2 07 54 pm

Unfortunately, enzyme does not support Portals so there is no way to test snapshots

@nreese nreese requested review from snide and cjcenizal January 2, 2018 21:13
@nreese nreese force-pushed the overlay_as_portal branch from 888c511 to 138694a Compare January 2, 2018 22:22
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Sweet!! I'm stoked that we're using the new portals. I had a few suggestions. Can we also add a note to the top of the component file about why there are no tests?

/**
 * NOTE: We can't test this component because Enzyme doesn't support rendering
 * into portals.
 */
 
import React, {
  Component,
} from 'react';

@@ -2,6 +2,8 @@ import React, {
Component,
} from 'react';
import PropTypes from 'prop-types';
import { createPortal } from 'react-dom';
import classnames from 'classnames';
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit but we've been aliasing this as classNames elsewhere.

className="euiOverlayMask"
{...rest}
/>
if (!this.overlayMaskNode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the docs recommend creating the node in the constructor and the using componentDidMount to attach it to the DOM. I think we can also do away with this check when we remove it, unless you found a case in which it didn't already exist?

import React, {
  Component,
} from 'react';
import PropTypes from 'prop-types';
import { createPortal } from 'react-dom';
import classNames from 'classnames';

export class EuiOverlayMask extends Component {
  constructor(props) {
    super(props);

    const {
      className,
      children,
      ...rest
    } = props;

    this.overlayMaskNode = document.createElement('div');
    this.overlayMaskNode.className = classNames(
      'euiOverlayMask',
      className
    );

    Object.keys(rest).forEach((key) => {
      this.overlayMaskNode.setAttribute(key, rest[key]);
    });
  }

  componentDidMount() {
    document.body.classList.add('euiBody-hasOverlayMask');
    document.body.appendChild(this.overlayMaskNode);
  }

  componentWillUnmount() {
    document.body.classList.remove('euiBody-hasOverlayMask');
    document.body.removeChild(this.overlayMaskNode);
    this.overlayMaskNode = null;
  }

  render() {
    return createPortal(
      this.props.children,
      this.overlayMaskNode
    );
  }
}

EuiOverlayMask.propTypes = {
  className: PropTypes.string,
  children: PropTypes.node,
};

@@ -34,7 +34,7 @@ export class ConfirmModal extends Component {

if (this.state.isModalVisible) {
modal = (
<EuiOverlayMask>
<EuiOverlayMask data-test-subj="exampleOverlayMask" className="test1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these attributes for the example?

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM!

@nreese nreese merged commit d5c7c83 into elastic:master Jan 3, 2018
@snide snide mentioned this pull request Jan 4, 2018
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.

3 participants