Implemented review comments

sulu · Aug 24, 2017 · 475b110 · 475b110
1 parent a844fa8
commit 475b110
Show file tree

Hide file tree

Showing 18 changed files with 167 additions and 140 deletions.
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/Backdrop.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/Backdrop.js
@@ -8,15 +8,16 @@ type Props = {
     isOpen: boolean,
     /** When set to false the backdrop renders transparent. */
     isVisible: boolean,
-    inPortal: boolean,
+    /** If true, the backdrop gets rendered in the placed element and not in the body. */
+    local: boolean,
     onClick?: () => void,
 };
 
 export default class Backdrop extends React.PureComponent<Props> {
     static defaultProps = {
         isOpen: true,
         isVisible: true,
-        inPortal: true,
+        local: false,
     };
 
     handleClick = () => {
@@ -35,7 +36,7 @@ export default class Backdrop extends React.PureComponent<Props> {
             [backdropStyles.isVisible]: isVisible,
         });
         const backdrop = <div onClick={this.handleClick} className={backdropClasses} />;
-        if (!this.props.inPortal) {
+        if (this.props.local) {
             return backdrop;
         }
         return <Portal isOpened={isOpen}>{backdrop}</Portal>;

diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/README.md b/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})} />
 </div>
 ```
 
@@ -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})} />
 </div>
 ```
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/tests/Backdrop.test.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Backdrop/tests/Backdrop.test.js
@@ -14,7 +14,7 @@ test('The component should render in body when open', () => {
 });
 
 test('The component should not render in body when property is set', () => {
-    const view = mount(<Backdrop inPortal={false} />).render();
+    const view = mount(<Backdrop local={true} />).render();
     expect(view).toMatchSnapshot();
 });
 

diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Icon/Icon.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Icon/Icon.js
@@ -5,16 +5,17 @@ import classNames from 'classnames';
 
 type Props = {
     className?: string,
+    onClick?: () => void,
     name: string,
 };
 
 export default class Icon extends React.PureComponent<Props> {
     render() {
-        const {className, name, ...otherProps} = this.props;
+        const {className, name, onClick} = this.props;
         const classes = classNames(className, 'fa', 'fa-' + name);
 
         return (
-            <span className={classes} aria-hidden={true} {...otherProps} />
+            <span className={classes} aria-hidden={true} onClick={onClick} />
         );
     }
 }
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Icon/tests/Icon.test.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Icon/tests/Icon.test.js
@@ -1,6 +1,6 @@
 /* eslint-disable flowtype/require-valid-file-annotation */
 import React from 'react';
-import {render} from 'enzyme';
+import {render, shallow} from 'enzyme';
 import Icon from '../Icon';
 
 test('Icon should render', () => {
@@ -10,3 +10,10 @@ test('Icon should render', () => {
 test('Icon should render with class names', () => {
     expect(render(<Icon className="test" name="edit" />)).toMatchSnapshot();
 });
+
+test('Icon should call the callback on click', () => {
+    const onClick = jest.fn();
+    const icon = shallow(<Icon className="test" name="edit" onClick={onClick} />);
+    icon.simulate('click');
+    expect(onClick).toBeCalled();
+});
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Actions.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Actions.js
@@ -0,0 +1,30 @@
+// @flow
+import React from 'react';
+import type {Action} from './types';
+import actionsStyles from './actions.scss';
+
+type Props = {
+    actions: Array<Action>,
+};
+
+export default class Actions extends React.PureComponent<Props> {
+    render() {
+        if (this.props.actions.length > 0) {
+            return (
+                <div className={actionsStyles.actions}>
+                    {this.props.actions.map((action, index) => {
+                        const handleButtonClick = action.onClick;
+                        return (
+                            <button
+                                key={index}
+                                className={actionsStyles.action}
+                                onClick={handleButtonClick}>{action.title}</button>
+                        );
+                    })}
+                </div>
+            );
+        }
+
+        return null;
+    }
+}
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Modal.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/Modal.js
@@ -72,27 +72,26 @@ export default class Modal extends React.PureComponent<Props> {
             [modalStyles.container]: true,
             [modalStyles.isDown]: this.isVisible,
         });
+        const {isOpen, title, actions, onRequestClose, onConfirm, confirmText, children} = this.props;
 
         return (
-            <div>
-                <Portal isOpened={this.props.isOpen || this.isOpenHasChanged}>
-                    <div
-                        className={containerClasses}
-                        onTransitionEnd={this.handleTransitionEnd} >
-                        <div className={modalStyles.box}>
-                            <ModalBox
-                                title={this.props.title}
-                                actions={this.props.actions}
-                                onRequestClose={this.props.onRequestClose}
-                                onConfirm={this.props.onConfirm}
-                                confirmText={this.props.confirmText} >
-                                {this.props.children}
-                            </ModalBox>
-                        </div>
-                        <Backdrop inPortal={false} onClick={this.props.onRequestClose} />
+            <Portal isOpened={isOpen || this.isOpenHasChanged}>
+                <div
+                    className={containerClasses}
+                    onTransitionEnd={this.handleTransitionEnd}>
+                    <div className={modalStyles.box}>
+                        <ModalBox
+                            title={title}
+                            actions={actions}
+                            onRequestClose={onRequestClose}
+                            onConfirm={onConfirm}
+                            confirmText={confirmText}>
+                            {children}
+                        </ModalBox>
                     </div>
-                </Portal>
-            </div>
+                    <Backdrop local={true} onClick={this.props.onRequestClose} />
+                </div>
+            </Portal>
         );
     }
 }
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/ModalBox.js
@@ -1,7 +1,8 @@
 // @flow
 import React from 'react';
 import Icon from '../Icon';
-import type {ModalProps, Action} from './types';
+import type {ModalProps} from './types';
+import Actions from './Actions';
 import modalBoxStyles from './modalBox.scss';
 
 type Props = ModalProps & {
@@ -18,42 +19,23 @@ export default class ModalBox extends React.PureComponent<Props> {
     render() {
         return (
             <div className={modalBoxStyles.box}>
-                <div className={modalBoxStyles.header}>
+                <header>
                     {this.props.title}
                     <Icon
                         name={CLOSE_ICON}
                         className={modalBoxStyles.icon}
                         onClick={this.props.onRequestClose} />
-                </div>
-                <div className={modalBoxStyles.content}>
+                </header>
+                <section>
                     {this.props.children}
-                </div>
-                <div className={modalBoxStyles.footer}>
-                    {ModalBox.renderActions(this.props.actions)}
+                </section>
+                <footer>
+                    <Actions actions={this.props.actions} />
                     <button
                         className={modalBoxStyles.confirmButton}
                         onClick={this.props.onConfirm}>{this.props.confirmText}</button>
-                </div>
+                </footer>
             </div>
         );
     }
-
-    static renderActions(actions: Array<Action>) {
-        if (actions.length > 0) {
-            return (
-                <div className={modalBoxStyles.actions}>
-                    {actions.map(ModalBox.renderAction)}
-                </div>
-            );
-        }
-    }
-
-    static renderAction(action: Action, index: number) {
-        return (
-            <button
-                key={index}
-                className={modalBoxStyles.action}
-                onClick={action.handleAction}>{action.title}</button>
-        );
-    }
 }
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/README.md b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/README.md
@@ -4,8 +4,8 @@ It renders depending on the passed property and request being closed through a c
 ```
 initialState = {open: false};
 const actions = [
-    {title: 'Destroy world', handleAction: () => {/* destroy world */}},
-    {title: 'Save world', handleAction: () => {/* save world */}},
+    {title: 'Destroy world', onClick: () => {/* destroy world */}},
+    {title: 'Save world', onClick: () => {/* save world */}},
 ];
 
 <div>
@@ -31,7 +31,7 @@ of the modal internally.
 ```
 const ClickModal = require('./ClickModal').default;
 const actions = [
-    {title: 'Save Gotham', handleAction: () => {/* save gotham */}},
+    {title: 'Save Gotham', onClick: () => {/* save gotham */}},
 ];
 
 const button = (<button>Open modal</button>);

diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/actions.scss b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/actions.scss
@@ -0,0 +1,20 @@
+@import '../../containers/Application/colors.scss';
+
+$actionFontSize: 14px;
+$actionColor: $black;
+$height: 90px;
+
+.actions {
+    line-height: $height;
+}
+
+.action {
+    padding: 0;
+    margin: 0 10px 0 0;
+    border: 0;
+    background: transparent;
+    cursor: pointer;
+    text-decoration: underline;
+    font-size: $actionFontSize;
+    color: $actionColor;
+}
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/modalBox.scss b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/modalBox.scss
@@ -14,9 +14,6 @@ $titleSize: 22px;
 $iconFontSize: 14px;
 $iconSize: ($iconFontSize + 10px);
 
-$actionFontSize: 14px;
-$actionColor: $black;
-
 $confirmBackground: $shakespeare;
 $confirmColor: $white;
 $confirmFontSize: 12px;
@@ -28,7 +25,7 @@ $confirmHeight: 30px;
     overflow: hidden;
     box-shadow: $boxShadow;
 
-    .header {
+    header {
         background: $secondaryBackground;
         height: $headerHeight;
         color: $titleColor;
@@ -38,13 +35,13 @@ $confirmHeight: 30px;
         line-height: $headerHeight;
     }
 
-    .content {
+    section {
         max-height: calc(100vh - $headerHeight - $footerHeight);
         overflow: auto;
         background: $primaryBackground;
     }
 
-    .footer {
+    footer {
         background: $secondaryBackground;
         height: $footerHeight;
         padding: 0 60px;
@@ -66,21 +63,6 @@ $confirmHeight: 30px;
     }
 }
 
-.actions {
-    line-height: $footerHeight;
-}
-
-.action {
-    padding: 0;
-    margin: 0 10px 0 0;
-    border: 0;
-    background: transparent;
-    cursor: pointer;
-    text-decoration: underline;
-    font-size: $actionFontSize;
-    color: $actionColor;
-}
-
 .confirm-button {
     background: $confirmBackground;
     color: $confirmColor;

diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/tests/Actions.test.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/tests/Actions.test.js
@@ -0,0 +1,24 @@
+/* eslint-disable flowtype/require-valid-file-annotation */
+import React from 'react';
+import {render, shallow} from 'enzyme';
+import Actions from '../Actions';
+
+test('The component should render', () => {
+    const actions = [
+        {title: 'Action 1', onClick: () => {}},
+        {title: 'Action 2', onClick: () => {}},
+    ];
+    const component = render(<Actions actions={actions} />);
+    expect(component).toMatchSnapshot();
+});
+
+test('The component should call the corresponding callback when an action is clicked', () => {
+    const actions = [
+        {title: 'Action 1', onClick: jest.fn()},
+        {title: 'Action 2', onClick: jest.fn()},
+    ];
+    const component = shallow(<Actions actions={actions} />);
+    component.find('button').first().simulate('click');
+    expect(actions[0].onClick).toBeCalled();
+    expect(actions[1].onClick).not.toBeCalled();
+});
diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/tests/ModalBox.test.js b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/tests/ModalBox.test.js
@@ -7,8 +7,8 @@ test('The component should render', () => {
     const onRequestClose = () => {};
     const onConfirm = () => {};
     const actions = [
-        {title: 'Action 1', handleAction: () => {}},
-        {title: 'Action 2', handleAction: () => {}},
+        {title: 'Action 1', onClick: () => {}},
+        {title: 'Action 2', onClick: () => {}},
     ];
     const box = render(
         <ModalBox
@@ -54,25 +54,3 @@ test('The component should call the callback when the confirm button is clicked'
     box.find('.confirmButton').simulate('click');
     expect(onConfirm).toBeCalled();
 });
-
-test('The component should call the corresponding callback when an action is clicked', () => {
-    const onRequestClose = () => {};
-    const onConfirm = () => {};
-    const actions = [
-        {title: 'Action 1', handleAction: jest.fn()},
-        {title: 'Action 2', handleAction: jest.fn()},
-    ];
-    const box = shallow(
-        <ModalBox
-            title="My title"
-            actions={actions}
-            onRequestClose={onRequestClose}
-            onConfirm={onConfirm}
-            confirmText="Alright mate!" >
-            <p>My modal content</p>
-        </ModalBox>
-    );
-    box.find('.actions button').first().simulate('click');
-    expect(actions[0].handleAction).toBeCalled();
-    expect(actions[1].handleAction).not.toBeCalled();
-});