feat(Modal): Create a Modal using shorthand props #1508

josie11 · 2017-03-25T12:57:01Z

can pass modal header, content, and actions via shorthand props (#1390)
can pass actions props a triggerClose option that will allow buttons to control open/closing modal

codecov-io · 2017-03-25T13:02:47Z

Codecov Report

Merging #1508 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1508      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         141      141              
  Lines        2384     2396      +12     
==========================================
+ Hits         2378     2390      +12     
  Misses          6        6

Impacted Files	Coverage Δ
src/modules/Modal/ModalActions.js	`100% <100%> (ø)`	⬆️
src/modules/Modal/Modal.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 573a0da...09db74c. Read the comment docs.

levithomason

Thanks for the work! Here's an initial pass review on progress so far.

levithomason · 2017-03-25T16:09:14Z

src/modules/Modal/Modal.js

@@ -265,6 +279,18 @@ class Modal extends Component {
      <ElementType {...rest} className={classes} style={{ marginTop }} ref={this.handleRef}>
        {Icon.create(closeIconName, { onClick: this.handleClose })}
        {children}
+        {_.isNil(children) && header && ModalHeader.create(header)}


We can remove the && header check for each shorthand create method as the factories handle falsy values internally.

levithomason · 2017-03-25T17:10:33Z

src/modules/Modal/ModalActions.js

@@ -37,4 +38,6 @@ ModalActions.propTypes = {
  className: PropTypes.string,
 }

+ModalActions.create = createShorthandFactory(ModalActions, children => ({ children }))


Because there can be many buttons, we need to allow the user to pass a key or our childKey prop. The third parameter to acreateShorthandFactory function is a boolean telling the factory to accept keys:

createShorthandFactory(ModalActions, children => ({ children }), true)

Next, this should probably map its value to an actions prop rather than children. The render method will then map over the actions and create buttons:

createShorthandFactory(ModalActions, actions => ({ actions }), true)

This enables users to use the actions prop on the Modal.Actions themselves as well:

<Modal> <Modal.Actions actions={['No', { primary: true, content: 'Yes' }]} /> </Modal>

levithomason · 2017-03-25T17:13:25Z

src/modules/Modal/Modal.js

+            }
+            action.onClick = onClick(action.onClick)
+          }
+          return Button.create(action, { key: i })


Using index for a key is an antipattern. Instead, we can configure the factory to allow the to pass a key. See comment on ModalActions.create below.

levithomason · 2017-03-25T17:18:36Z

src/modules/Modal/Modal.js

@@ -53,15 +57,21 @@ class Modal extends Component {
    /** Whether or not the Modal should close when the document is clicked. */
    closeOnDocumentClick: PropTypes.bool,

+    /** Simple text content for the Modal. */
+    content: customPropTypes.itemShorthand,


As we update propTypes, we should also update the typings in the same directory. Example, Modal.d.ts.

…rop for action in Modal Action to fix failing test

josie11 · 2017-03-27T21:28:25Z

I've made the changes.

layershifter · 2017-03-28T07:18:47Z

src/modules/Modal/ModalActions.js

@@ -27,6 +35,9 @@ ModalActions._meta = {
 }

 ModalActions.propTypes = {
+  /** Elements to render as Modal action buttons. */
+  actions: PropTypes.arrayOf(customPropTypes.itemShorthand),


as is always first in propTypes

layershifter · 2017-03-28T07:19:19Z

src/modules/Modal/ModalActions.js


 /**
 * A modal can contain a row of actions.
 */
 function ModalActions(props) {
-  const { children, className } = props
+  const { children, className, actions } = props


Please sort props in alphabetical order

layershifter · 2017-03-28T07:22:13Z

src/modules/Modal/ModalActions.js

+      {children}
+      {_.isNil(children) && actions.map((action) => Button.create(action, true))}
+    </ElementType>
+  )


I think we can make there a short condition:

if(!_.isNil(children)) return <ElementType {...rest} className={classes}>{children}</ElementType> return ( <ElementType {...rest} className={classes}> {actions.map(action => Button.create(action, true))} </ElementType> )

layershifter · 2017-03-28T07:24:28Z

src/modules/Modal/Modal.js

+              if (callback) callback(e, this.props)
+              this.handleClose()
+            }
+            action.onClick = onClick(action.onClick)


Can we move event handler to a class method?

josie11 · 2017-03-28T14:54:55Z

I've made the changes, and I added an example.

levithomason

Here is another interim review. We're getting closer here, thanks for continuing on this!

levithomason · 2017-03-28T23:01:05Z

docs/app/Examples/modules/Modal/Types/ModalExampleShorthand.js

+import { Button, Modal } from 'semantic-ui-react'
+
+const ModalShorthandExample = () => (
+  <Modal trigger={<Button>Show Modal</Button>}


trigger on new line please.

levithomason · 2017-03-28T23:08:17Z

src/modules/Modal/ModalActions.js

+
+  return (
+    <ElementType {...rest} className={classes}>
+      {actions.map((action) => Button.create(action, true))}


Ah, this is a new use case. We currently don't have a way to enable keys for a create method during runtime like this. Instead, the create method itself is defined as supporting keys or not. The button currently does not support keys.

The second param here is actually defaultProps. In order to support keys, we need to pass true as the 3rd param to createShorthandFactory for the Button's create method:

-Button.create = createShorthandFactory(Button, value => ({ content: value })) +Button.create = createShorthandFactory(Button, value => ({ content: value }), true)

We have a PR up for factory updates. I'll enable keys for all factories in light of this issue. We want users to be able to use the create method themselves as well, and, they may need keys.

levithomason · 2017-03-28T23:16:28Z

src/modules/Modal/ModalActions.js

+  actions: [],
+}
+
+ModalActions.create = createShorthandFactory(ModalActions, actions => ({ actions }), true)


Because there will only be one instance of Modal.Actions, the Modal.Actions component itself won't need to have keys. We can remove true from here.

See above comment regarding Button.create which is what will need the keys, since, there is an array of button elements being created (within Modal.Actions).

josie11 · 2017-03-29T13:48:15Z

Changes are made - I've enjoyed learning whats going on under the hood a bit more.

update branch to master

levithomason

Just one change on this review. I think we're ready to get some tests in here as well 👍

We have a common test for testing shorthand props. Search for implementsShorthandProp in other tests to see how it works. You'll have a bit of a tricky case with actions when using triggerClose as this is not a Button prop and needs to be removed from the object.

levithomason · 2017-03-30T20:39:03Z

src/modules/Modal/ModalActions.js

  /** Primary content. */
  children: PropTypes.node,

  /** Additional classes. */
  className: PropTypes.string,
 }

+ModalActions.defaultProps = {
+  actions: [],
+}


Since shorthand is disallowed with children, we'll want to avoid setting default values for it. Otherwise, when a user uses children they will also have conflicting actions array defined unawares.

Instead, in the render method, just use _.map which will gracefully handle undefined values:

-{actions.map((action) => Button.create(action))} +{_.map(actions, action => Button.create(action))}

josie11 · 2017-03-31T03:21:21Z

I added some tests to Modal. Should the situation with triggerClose/Button be throwing an error? I could prevent triggerClose from being passed to ModalActions by using _.omit(action, 'triggerClose') and not using _.forEach.

I had to add _.isPlanObject(action) to prevent implementsShorthandProp from failing for actions:

{_.isNil(children) && ModalActions.create(_.forEach(actions, (action) => {
  if (_.isPlainObject(action) && action.triggerClose) action.onClick this.handleTriggerClose(action.onClick)
}))}

josie11 · 2017-04-06T16:30:07Z

Is there anything I should change/fix here?

…React into modal

layershifter

@josie11 Thanks for PR, again 👍 I shipped some updates there. I'll finish review tomorrow, need more time on tests

levithomason · 2017-04-07T16:26:27Z

Thanks, folks! As a heads up, I'll be out on travel for a week starting today. Expect silence from me during this time, however, I will try to catch some mobile updates as I am able.

layershifter

I've updated tests. @levithomason we need your review there 😄

…React into modal # Conflicts: # src/modules/Modal/ModalActions.js

layershifter · 2017-04-17T12:18:24Z

@levithomason I've resolved merge conflicts, will be glad to receive your review there 👍

levithomason · 2017-04-18T17:00:28Z

Changes look great, merging!

levithomason · 2017-04-22T17:02:33Z

Released in semantic-ui-react@0.68.1.

josie11 added 4 commits March 23, 2017 11:23

implement modal shorthand functionality

1c75f66

remove and refactor a few lines

517cc88

delete redundant line

2f1ff84

Merge branch 'master' into modal

753bfa2

levithomason mentioned this pull request Mar 25, 2017

feat(Portal|Modal|Confirm): Better uncontrolled usage support #893

Closed

levithomason requested changes Mar 25, 2017

View reviewed changes

levithomason added the needs author feedback label Mar 25, 2017

josie11 added 4 commits March 25, 2017 21:01

Make requested changes to PR

3b1f6a7

Remove import of Button in Modal

38145d4

add lodash import to modal actions file

f99337d

add implementsCreateShortMethod to Modal Actions tests, add default p…

2366f96

…rop for action in Modal Action to fix failing test

layershifter reviewed Mar 28, 2017

View reviewed changes

layershifter requested changes Mar 28, 2017

View reviewed changes

josie11 added 2 commits March 28, 2017 10:13

Make requested changes to PR

394ab3f

add example to modal examples

1bb7927

fix linting error

bf7a411

levithomason requested changes Mar 28, 2017

View reviewed changes

make request chages to PR

48ee38b

Merge remote-tracking branch 'origin' into modal

bad477d

update branch to master

levithomason reviewed Mar 30, 2017

View reviewed changes

add tests to Modal for shorthand props

e2f8e30

minor adjustment to tests

512fb7e

Alexander Fedyashov added 2 commits April 7, 2017 18:34

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

d3a9dc3

…React into modal

wip(ModalActions): update event handling

cb58917

layershifter reviewed Apr 7, 2017

View reviewed changes

josie11 and others added 2 commits April 7, 2017 13:07

Add missing ! to isNil

75a0a5a

tests(mixed): update tests

f7e59c8

layershifter approved these changes Apr 9, 2017

View reviewed changes

layershifter added ready for review and removed needs author feedback labels Apr 9, 2017

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

76bfa3a

…React into modal # Conflicts: # src/modules/Modal/ModalActions.js

fix(ModalAction): add Component to imports

09db74c

levithomason approved these changes Apr 18, 2017

View reviewed changes

levithomason merged commit c397f3e into Semantic-Org:master Apr 18, 2017

josie11 deleted the modal branch April 18, 2017 17:51

layershifter mentioned this pull request Apr 22, 2017

fix(Modal): make content and header props optional #1609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Modal): Create a Modal using shorthand props #1508

feat(Modal): Create a Modal using shorthand props #1508

josie11 commented Mar 25, 2017 •

edited

Loading

codecov-io commented Mar 25, 2017 •

edited

Loading

levithomason left a comment

levithomason Mar 25, 2017

levithomason Mar 25, 2017

levithomason Mar 25, 2017

levithomason Mar 25, 2017

josie11 commented Mar 27, 2017

layershifter Mar 28, 2017

layershifter Mar 28, 2017

layershifter Mar 28, 2017

layershifter Mar 28, 2017

josie11 commented Mar 28, 2017

levithomason left a comment

levithomason Mar 28, 2017

levithomason Mar 28, 2017

levithomason Mar 28, 2017

josie11 commented Mar 29, 2017

levithomason left a comment

levithomason Mar 30, 2017

josie11 commented Mar 31, 2017 •

edited

Loading

josie11 commented Apr 6, 2017

layershifter left a comment

levithomason commented Apr 7, 2017

layershifter left a comment

layershifter commented Apr 17, 2017

levithomason commented Apr 18, 2017

levithomason commented Apr 22, 2017

feat(Modal): Create a Modal using shorthand props #1508

feat(Modal): Create a Modal using shorthand props #1508

Conversation

josie11 commented Mar 25, 2017 • edited Loading

codecov-io commented Mar 25, 2017 • edited Loading

Codecov Report

levithomason left a 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

josie11 commented Mar 27, 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

josie11 commented Mar 28, 2017

levithomason left a 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

josie11 commented Mar 29, 2017

levithomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josie11 commented Mar 31, 2017 • edited Loading

josie11 commented Apr 6, 2017

layershifter left a comment

Choose a reason for hiding this comment

levithomason commented Apr 7, 2017

layershifter left a comment

Choose a reason for hiding this comment

layershifter commented Apr 17, 2017

levithomason commented Apr 18, 2017

levithomason commented Apr 22, 2017

josie11 commented Mar 25, 2017 •

edited

Loading

codecov-io commented Mar 25, 2017 •

edited

Loading

josie11 commented Mar 31, 2017 •

edited

Loading