Implemented review comments

sulu · Aug 24, 2017 · 57ab1c7 · 57ab1c7
1 parent a844fa8
commit 57ab1c7
Show file tree

Hide file tree

Showing 11 changed files with 64 additions and 73 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/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
@@ -18,22 +18,22 @@ 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}>
+                </section>
+                <footer>
                     {ModalBox.renderActions(this.props.actions)}
                     <button
                         className={modalBoxStyles.confirmButton}
                         onClick={this.props.onConfirm}>{this.props.confirmText}</button>
-                </div>
+                </footer>
             </div>
         );
     }

diff --git a/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/modalBox.scss b/src/Sulu/Bundle/AdminBundle/Resources/js/components/Modal/modalBox.scss
@@ -28,7 +28,7 @@ $confirmHeight: 30px;
     overflow: hidden;
     box-shadow: $boxShadow;
 
-    .header {
+    header {
         background: $secondaryBackground;
         height: $headerHeight;
         color: $titleColor;
@@ -38,13 +38,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;

diff --git a/...dle/AdminBundle/Resources/js/components/Modal/tests/__snapshots__/ClickModal.test.js.snap b/...dle/AdminBundle/Resources/js/components/Modal/tests/__snapshots__/ClickModal.test.js.snap
@@ -5,15 +5,15 @@ exports[`The modal should be rendered when the button got clicked 1`] = `
   <div data-reactroot=\\"\\" class=\\"container isDown\\">
     <div class=\\"box\\">
       <div class=\\"box\\">
-        <div class=\\"header\\">
+        <header>
           <!-- react-text: 5 -->My modal title
-          <!-- /react-text --><span class=\\"icon fa fa-times\\" aria-hidden=\\"true\\"></span></div>
-        <div class=\\"content\\">
+          <!-- /react-text --><span class=\\"icon fa fa-times\\" aria-hidden=\\"true\\"></span></header>
+        <section>
           <p>My modal content</p>
-        </div>
-        <div class=\\"footer\\">
+        </section>
+        <footer>
           <button class=\\"confirmButton\\">Apply</button>
-        </div>
+        </footer>
       </div>
     </div>
     <div class=\\"backdrop isVisible\\"></div>
@@ -26,8 +26,5 @@ exports[`The modal should initially not be rendered 1`] = `
   <button>
     Open modal
   </button>
-  <div>
-    
-  </div>
 </div>
 `;
diff --git a/...u/Bundle/AdminBundle/Resources/js/components/Modal/tests/__snapshots__/Modal.test.js.snap b/...u/Bundle/AdminBundle/Resources/js/components/Modal/tests/__snapshots__/Modal.test.js.snap
@@ -1,31 +1,23 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`The component should not render in body when closed 1`] = `
-<div>
-  
-</div>
-`;
+exports[`The component should not render in body when closed 1`] = `null`;
 
-exports[`The component should render in body when open 1`] = `
-<div>
-  
-</div>
-`;
+exports[`The component should render in body when open 1`] = `null`;
 
 exports[`The component should render in body when open 2`] = `
 "<div>
   <div data-reactroot=\\"\\" class=\\"container\\">
     <div class=\\"box\\">
       <div class=\\"box\\">
-        <div class=\\"header\\">
+        <header>
           <!-- react-text: 5 -->My modal title
-          <!-- /react-text --><span class=\\"icon fa fa-times\\" aria-hidden=\\"true\\"></span></div>
-        <div class=\\"content\\">
+          <!-- /react-text --><span class=\\"icon fa fa-times\\" aria-hidden=\\"true\\"></span></header>
+        <section>
           <p>My modal content</p>
-        </div>
-        <div class=\\"footer\\">
+        </section>
+        <footer>
           <button class=\\"confirmButton\\">Apply</button>
-        </div>
+        </footer>
       </div>
     </div>
     <div class=\\"backdrop isVisible\\"></div>

diff --git a/...undle/AdminBundle/Resources/js/components/Modal/tests/__snapshots__/ModalBox.test.js.snap b/...undle/AdminBundle/Resources/js/components/Modal/tests/__snapshots__/ModalBox.test.js.snap
@@ -4,25 +4,19 @@ exports[`The component should render 1`] = `
 <div
   class="box"
 >
-  <div
-    class="header"
-  >
+  <header>
     My title
     <span
       aria-hidden="true"
       class="icon fa fa-times"
     />
-  </div>
-  <div
-    class="content"
-  >
+  </header>
+  <section>
     <p>
       My modal content
     </p>
-  </div>
-  <div
-    class="footer"
-  >
+  </section>
+  <footer>
     <div
       class="actions"
     >
@@ -42,6 +36,6 @@ exports[`The component should render 1`] = `
     >
       Alright mate!
     </button>
-  </div>
+  </footer>
 </div>
 `;