Modal: Height is bigger than needed and tooltip is wrong positioned

[mmtr]

Describe the bug
When rendering a Modal component outside Gutenberg, the height of the component is bigger than the one needed for fitting the content.

Also, the tooltip related to the Close dialog button is wrong positioned.

To Reproduce
Execute the code below in a new React project after installing @wordpress/components and @wordpress/compose.

import React from 'react';
import ReactDOM from 'react-dom';
import { Button, Modal } from '@wordpress/components';
import { withState } from '@wordpress/compose';

import '@wordpress/components/build-style/styles.css';

const MyModal = withState( {
	isOpen: false,
} )( ( { isOpen, setState } ) => (
	<div>
		<Button isDefault onClick={ () => setState( { isOpen: true } ) }>Open Modal</Button>
		{ isOpen ?
			<Modal
				title="This is my modal"
				onRequestClose={ () => setState( { isOpen: false } ) }>
				<Button isDefault onClick={ () => setState( { isOpen: false } ) }>
					My custom close button
				</Button>
			</Modal> 
			: null }
	</div>
) );

ReactDOM.render(
	<MyModal />,
	document.getElementById( 'root' )
);

Expected behavior
The modal height should be the minimum needed for fitting the content. The tooltip message that appears when hovering the close button should appear above the button.

Screenshots

Desktop:

• OS: macOS High Sierra
• Browser: Chrome
• Version: 67

Additional context
Issue found while working on #8338 and Automattic/wp-calypso#26367

[abotteram]

@mmtr The height issue can easily be fixed by yourself. If this is something we want to change in gutenberg is something @jasmussen should say.

Add a className prop, for example my-wicked-modal and create a css group:

.my-wicked-modal.my-wicked-modal /* workaround for specificity issues */ {
    height: initial;
    min-height: 50px;
}

I'm not able to reproduce the tooltip issue though.

[mmtr]

@xyfi Thanks for suggestion for fixing the height issue!

I'm wondering if this height is the default one the package should provide or is it better if the height doesn't exceed by default the needed one by the content.

Regarding the tooltip issue, you can reproduce it here:

• Demo: https://mmtr.github.io/wordpress-components/#/modal
• Code: https://github.com/mmtr/wordpress-components/blob/master/src/examples/modal.js

[jasmussen]

I think it's important to have good defaults, and right now the modal is designed for lots of real estate. I think it might be good that modals are born with the same dimensions for consistency.

When I looked at the modal code, I recall there being some challenges with making the dimensions fit the content. Remember, this has to work across mobile, small tablets up to desktops, including big screens. That doesn't mean it's not possible, just that I recall this being difficult to pull off. I may be wrong.

I know that Matías had an idea to add a isCompact property to the modal component, which would open a minimal modal (something close to an alert) as opposed to the big one we have now. I also personally wouldn't object to adding a width/height prop that was available to plugin devs, if we can't make the height smart, that is.

[afercia]

Re: the height issue: height: initial or better auto can make the modal content not scrollable in landscape mode (just faced this issue in a custom implementation).

I'd tend to think the height thing should be reviewed. Instead of setting it explicitly, the height should be determined by the content. A max-height value can then be used in the media queries to constrain the modal in the viewport. Then transform: translate(-50%, 0) should take care of centering vertically the modal (it's based on the element intrinsic size). Something along these lines should work:

left: 50%;
top: 50%;
max-height: { as desired };
transform: translate(-50%, -50%);
height: auto: <-- just for clarification, can be omitted

[afercia]

Hm probably also the scrolling mechanism of the modal content should be reviewed.

[tn3rb]

tooltip position bug appears to be identical to the one i reported in #8468