Add overlay component #3466

mmsbrggr · 2017-08-03T07:01:06Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Dependent PR	#3468
License	MIT

What's in this PR?

This PR contains a modal components which displays content above everything else.

Example Usage

<button onClick={() => setState({open: true})}>Open overlay</button>
<Overlay onClose={() => setState({open: false})} isOpen={state.open}>
    <p>My overlay content</p>
</Overlay >

mmsbrggr · 2017-08-03T07:14:45Z

src/Sulu/Bundle/AdminBundle/Resources/js/package.json

@@ -27,6 +28,7 @@
        "enzyme-to-json": "^1.5.1",
        "identity-obj-proxy": "^3.0.0",
        "jest": "^20.0.4",
+        "pretty": "^2.0.0",


pretty is just a dev-dependency which beautifies a string of html. This is used for testing the html of the overlay which is ported to the body.

danrot · 2017-08-03T08:15:47Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ClickModal.js

+import {observer} from 'mobx-react';
+
+@observer
+export default class ClickModal extends React.PureComponent {


I think I would wait with the merge until we know how we name this.

danrot · 2017-08-03T08:16:43Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ClickModal.js

+    render() {
+        return (
+            <div className={this.props.className}>
+                {React.cloneElement(this.props.clickElement, {onClick: this.handleElementClick})}


Why do we need to clone something here?

To pass an extra property

danrot · 2017-08-03T08:16:55Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ClickModal.js

+        this.modalOpen = true;
+    };
+
+    @action handleModalRequestClose = () => {


Just handleRequestClose?

danrot · 2017-08-03T08:20:58Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Modal.js

+                    <div className={modalStyle.modal}>
+                        {this.props.children}
+                    </div>
+                    <div onClick={this.handleBackdropClick} className={modalStyle.backdrop} />


Wasn't the idea to move that into an own Backdrop component, so that it can also be used for the dropdowns?

danrot · 2017-08-03T08:24:01Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/modal.scss

+$backgroundColor: $white;
+$modalShadow: 0 0 25px 0 rgba($black, 0.25);
+
+.container {


.modal-container

Isn't the major advantage of css-modules that we can use the same classes in multiple files without caution? modal is mentioned in the name of the file.

danrot · 2017-08-03T08:46:48Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/tests/ClickModal.test.js

+        </ClickModal>
+    ).render();
+    expect(view).toMatchSnapshot();
+    expect(body.innerHTML).toBe('');


Shouldn't that contain at least the button with the Open modal text? Or am I missing something.

The view is rendered in a virtual document as far as I know. Only the stuff in the portal is put in the body mentioned here.

danrot · 2017-08-03T08:47:40Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/tests/ClickModal.test.js

+
+    expect(body.innerHTML).toBe('');
+    view.find('button').simulate('click');
+    expect(pretty(body.innerHTML)).toMatchSnapshot();


Related to the question above: Don't you make a far to big snapshot this way?

As mentioned, the only thing in the body is the stuff of the portal and that's exactly the things a snapshot should be taken of.

danrot · 2017-08-03T08:48:07Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/tests/Modal.test.js

+    const body = document.body;
+    const view = mount(<Modal isOpen={true}><p>Modal content</p></Modal>).render();
+    expect(view.html()).toBe(null);
+    expect(pretty(body.innerHTML)).toMatchSnapshot();


Same as above

As mentioned, the only thing in the body is the stuff of the portal and that's exactly the things a snapshot should be taken of.

danrot · 2017-08-03T08:48:55Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/tests/Modal.test.js

+    const backdrop = view.find('.backdrop');
+    expect(backdrop.length).toBe(1);
+
+    expect(requestCloseSpy).toHaveBeenCalledTimes(0);


I have used toBeCalledTimes until now, which is an alias for that. Would be nice to be consistent here.

Where is this function situated? If I replace toHaveBeenCalledTimes with toBeCalledTimes, I get that toBeCalledTimes is not a function.

danrot · 2017-08-24T08:57:36Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/Backdrop.js

@@ -8,12 +8,15 @@ type Props = {
    isOpen: boolean,
    /** When set to false the backdrop renders transparent. */
    isVisible: boolean,
+    inPortal: boolean,


I don't really get what this prop is doing.

danrot · 2017-08-24T08:59:03Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/backdrop.scss

@@ -1,17 +1,16 @@
 @import '../../containers/Application/colors.scss';

-$backdropBackground: $black;
+$backdropBackground: rgba($dustyGrey, .9);


Was this color decided by the designers?

danrot · 2017-08-24T09:00:49Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Icon/Icon.js

-            'fa',
-            'fa-' + this.props.name
-        );
+        const {className, name, ...otherProps} = this.props;


What do you need the otherProps for?

create explicit onclick

danrot · 2017-08-24T09:05:16Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ClickModal.js

+};
+
+@observer
+export default class ClickModal extends React.PureComponent<Props> {


I am still not a big fan of this component... Don't you think it would make more sense to implement this as a high order component?

I can't see the advantages of using a HOC here.

danrot · 2017-08-24T09:09:57Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Modal.js

+    onRequestClose: () => void,
+};
+
+const ESC_KEY = 27;


I think that should go somewhere else, or we should include a library handling this for us. I don't want to define the ESC keycode in every component 😉

If I remember correctly @chirimoya found a react library handling that using decorators.

https://github.com/glortho/react-keydown ?

react-keydown only listens to keydowns within the component elements. Because we use portals in the modal it doesn't really work. I created an issue in react-keydown: glortho/react-keydown#68

danrot · 2017-08-24T09:16:22Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Modal.js

+                <Portal isOpened={this.props.isOpen || this.isOpenHasChanged}>
+                    <div
+                        className={containerClasses}
+                        onTransitionEnd={this.handleTransitionEnd} >


No space before >

danrot · 2017-08-24T09:17:13Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Modal.js

+                    <div
+                        className={containerClasses}
+                        onTransitionEnd={this.handleTransitionEnd} >
+                        <div className={modalStyles.box}>


Why not moving that class to the ModalBox?

Because it handles the transition. The modal box is just the element which doesn't care where its placed and how it appears.

danrot · 2017-08-24T09:17:49Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Modal.js

+                                actions={this.props.actions}
+                                onRequestClose={this.props.onRequestClose}
+                                onConfirm={this.props.onConfirm}
+                                confirmText={this.props.confirmText} >


use destructuring for all these variables.

danrot · 2017-08-24T09:19:21Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js

+                        className={modalBoxStyles.confirmButton}
+                        onClick={this.props.onConfirm}>{this.props.confirmText}</button>
+                </div>
+            </div>


I think we should make use of the section, header and footer tags here, shouldn't we?

And maybe also put them in separate components? Having a render* method is usually a sign that it might be better to put into a separate component.

danrot · 2017-08-24T09:28:55Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/types.js

+
+export type Action = {
+    title: string,
+    handleAction: () => void,


Why not onClick?

danrot · 2017-08-24T11:59:33Z

I think the name of the component is a bit controversial... We'll have two different kind of overlays, and both of them will be modals. But we are already occupying this term for one of these overlays...

chirimoya · 2017-08-24T12:09:19Z

@danrot are we not able to manage the different kind of overlays in one component?

mmsbrggr · 2017-08-24T15:12:21Z

@danrot The other kind of modal you talk about are "Dialogs" for me.

danrot · 2017-08-24T16:24:45Z

@chirimoya TBH I don't know yet... There are no designs 🙈 It's again the question if we build it using some configuration, or if the differences are so big that they justify a separate component.

@mmsbrggr Both of them are modals per definitions. A modal is something that requires immediate attention and blocks the rest of the application, which is true for both components (if there will be two)

danrot · 2017-08-24T16:26:36Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/README.md

@@ -7,7 +7,7 @@ intialState = {open: false};

 <div>
    <button onClick={() => setState({open: true})}>Open Backdrop</button>
-    <Backdrop isOpen={state.open} onClick={() => setState({open: false})} />
+    <Backdrop isOpen={!!state.open} onClick={() => setState({open: false})} />


Why the double bang?

Because initially the state.open is undefined in styleguidist. As the default value for isOpen is true the Backdrop opens automatically. That's why the backdrops were open on page load all the time.

danrot · 2017-08-24T16:26:52Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/README.md

@@ -18,6 +18,6 @@ intialState = {open: false};

 <div>
    <button onClick={() => setState({open: true})}>Open Backdrop</button>
-    <Backdrop isVisible={false} isOpen={state.open} onClick={() => setState({open: false})} />
+    <Backdrop isVisible={false} isOpen={!!state.open} onClick={() => setState({open: false})} />


Why the double bang? We actually said we don't do them anymore.

The make sense sometimes. Especially at the points where you really expect a boolean and not just a truthy type. At this point undefined and false make a difference.

danrot · 2017-08-24T16:27:22Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/tests/Backdrop.test.js

@@ -13,6 +13,11 @@ test('The component should render in body when open', () => {
    expect(pretty(body.innerHTML)).toMatchSnapshot();
 });

+test('The component should not render in body when property is set', () => {


when local property

danrot · 2017-08-24T16:33:35Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ClickModal.js

+                <Modal
+                    isOpen={this.modalOpen}
+                    onRequestClose={this.handleRequestClose}
+                    {...modalProps} >


No space before > (there has to be an eslint rule for that...)

danrot · 2017-08-24T16:36:53Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js

+                        className={modalBoxStyles.icon}
+                        onClick={this.props.onRequestClose} />
+                </header>
+                <section>


I would have left this one a div and the outer div would be the section for me, since a section is allowed to have headers and footers (if I am not mistaken).

danrot · 2017-08-24T16:37:07Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js

+        return (
+            <div className={modalBoxStyles.box}>
+                <header>
+                    {this.props.title}


use destructuring

danrot · 2017-08-25T07:03:48Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Actions.js

+
+export default class Actions extends React.PureComponent<Props> {
+    render() {
+        if (this.props.actions.length > 0) {


use destructuring? At first I thought it doesn't make any sense, but you are using it twice, so it does for me.

danrot · 2017-08-25T07:06:12Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js

+        actions: [],
+    };
+
+    @keydown('esc') requestClose() {


rename to handleRequestClose?

The component doesn't handle a request close call, but really requests to be closed.

danrot · 2017-08-25T07:07:38Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js

+            <section className={modalBoxStyles.box}>
+                <header>
+                    {title}
+                    <Icon name={CLOSE_ICON} className={modalBoxStyles.icon} onClick={onRequestClose} />


use requestClose resp. handleRequestClose directly? What if we have to do more than just calling one callback?

danrot · 2017-08-25T07:08:14Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/README.md

+        title="Njan Njan Njan"
+        onRequestClose={() => setState({open: false})}
+        actions={actions}
+        confirmText="Apply"


Maybe also add a onConfirm callback?

I added it to both examples

danrot · 2017-08-25T07:08:28Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/README.md

+    clickElement={button}
+    title="Nana Nana Nana"
+    actions={actions}
+    confirmText="Ok">


Also add a onConfirm callback?

mmsbrggr · 2017-08-25T07:16:20Z

@danrot I couldn't find a possibility to forbid spaces before ">" in the starting tag via es-lint. I created an issue jsx-eslint/eslint-plugin-react#1396

danrot · 2017-08-25T13:00:51Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Actions.js

+export default class Actions extends React.PureComponent<Props> {
+    render() {
+        const {actions} = this.props;
+        if (actions.length > 0) {


early return

danrot · 2017-08-28T06:19:32Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ClickModal.js

+};
+
+@observer
+export default class ClickModal extends React.PureComponent<Props> {


Rename to ClickOverlay?

danrot · 2017-08-28T06:19:52Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ClickModal.js

+
+@observer
+export default class ClickModal extends React.PureComponent<Props> {
+    @observable modalOpen = false;


Rename to isOpen?

danrot · 2017-08-28T06:25:48Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js

+
+const CLOSE_ICON = 'times';
+
+export default class ModalBox extends React.PureComponent<Props> {


OverlayBox? Box is strange somehow, because it doesn't transport a real message for me... Maybe there is a better alternative.

danrot · 2017-08-28T06:26:21Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js

+import Icon from '../Icon';
+import type {ModalProps} from './types';
+import Actions from './Actions';
+import modalBoxStyles from './modalBox.scss';


Rename to overlayBoxStyles and overlayBox.scss?

danrot · 2017-08-28T06:36:31Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/index.js

+import Modal from './Modal';
+
+export {ClickModal};
+export default Modal;


How should these imports be handled?

Move the ClickModal to its own component folder?

Have two named exports?

Decide on which one is the more "default" one, and the other is the named export?

Is there an actual need for both kind of modals? Could imagine that we always only open the modals when an element is clicked.

danrot · 2017-08-28T06:37:02Z

src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Modal.js

+import Backdrop from '../Backdrop';
+import ModalBox from './ModalBox';
+import type {ModalProps} from './types';
+import modalStyles from './modal.scss';


Rename to overlayStyles?

mmsbrggr changed the title ~~Feature/modal~~ Modal component Aug 3, 2017

mmsbrggr commented Aug 3, 2017

View reviewed changes

danrot suggested changes Aug 3, 2017

View reviewed changes

mmsbrggr force-pushed the feature/modal branch 2 times, most recently from 41f9dcf to 47ee3f7 Compare August 3, 2017 13:45

mmsbrggr force-pushed the feature/modal branch 7 times, most recently from 5894bb7 to a844fa8 Compare August 24, 2017 08:38

danrot suggested changes Aug 24, 2017

View reviewed changes

mmsbrggr force-pushed the feature/modal branch 2 times, most recently from 57ab1c7 to 475b110 Compare August 24, 2017 15:48

danrot suggested changes Aug 24, 2017

View reviewed changes

mmsbrggr force-pushed the feature/modal branch 4 times, most recently from 2fc28e0 to bbc5a24 Compare August 25, 2017 06:20

danrot suggested changes Aug 25, 2017

View reviewed changes

mmsbrggr force-pushed the feature/modal branch from 211a513 to 80989de Compare August 25, 2017 07:42

danrot suggested changes Aug 25, 2017

View reviewed changes

danrot suggested changes Aug 28, 2017

View reviewed changes

Created modal component

0c69657

mmsbrggr and others added 9 commits August 28, 2017 16:30

Wrote modal tests

9457190

Implemented review comments

57fb0f3

Refactored flow types

6bc6ed0

Implemented modal element

eb359dc

Implemented review comments

eab5d26

Added react-keydown for modal closing

296e089

Implemented review comments

aa65fce

use early return

62a5097

removed double colors

78ec0ea

danrot force-pushed the feature/modal branch from 8327662 to 78ec0ea Compare August 28, 2017 14:30

danrot added 5 commits August 28, 2017 16:34

remove ClickModal

afc0b32

renamed types to OverlayProps

6a36954

removed ModalBox

ff19ff0

renamed css

a5388cd

renamed modal folder to overlay

273688e

danrot changed the title ~~Modal component~~ Add overlay component Aug 28, 2017

add mousetrap for keyboard shortcut

e6753cb

danrot force-pushed the feature/modal branch from 42317be to e6753cb Compare August 29, 2017 06:54

danrot approved these changes Aug 29, 2017

View reviewed changes

renamed onRequestClose to requestClose

a391c12

danrot force-pushed the feature/modal branch from 97e10d9 to a391c12 Compare August 29, 2017 14:00

chirimoya merged commit 1b2f955 into sulu:develop Aug 29, 2017


		const CLOSE_ICON = 'times';

		export default class ModalBox extends React.PureComponent<Props> {

Add overlay component #3466

Add overlay component #3466

Conversation

mmsbrggr commented Aug 3, 2017 • edited by chirimoya

What's in this PR?

Example Usage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmsbrggr Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danrot commented Aug 24, 2017

chirimoya commented Aug 24, 2017

mmsbrggr commented Aug 24, 2017

danrot commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmsbrggr commented Aug 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmsbrggr commented Aug 3, 2017 •

edited by chirimoya

mmsbrggr Aug 24, 2017 •

edited

mmsbrggr commented Aug 25, 2017 •

edited